Closed Bug 1490119 Opened 6 years ago Closed 6 years ago

make update verify work in staging with dep signing

Categories

(Release Engineering :: Release Automation: Updates, enhancement, P1)

Product:
Release Engineering
Component:
Release Automation: Updates
Type:
enhancement

Tracking

(firefox-esr60 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed

People

(Reporter: bhearsum, Unassigned)

References

Details

(Keywords: leave-open)

Attachments

(6 files, 5 obsolete files)

bug 1490119: Add build system bits for building dep updater.
(deleted), text/x-phabricator-request
robert.strong.bugs
: review+
Details
Set override_certs in update verify config creator.
(deleted), text/x-phabricator-request
nthomas
: review+
Details
tools patch, as landed
(deleted), patch
bhearsum
: checked-in+
Details | Diff | Splinter Review
bug 1490119: Add build system bits for building dep updater.
(deleted), text/x-phabricator-request
robert.strong.bugs
: review+
mshal
: review+
Details
Bump timeout for automationrelevance.
(deleted), text/x-phabricator-request
tomprince
: review+
Details
bug 1490119: Build separate updater that always embeds dep certificates.
(deleted), text/x-phabricator-request
ted
: review+
Details
We've had a long standing problem in staging releases where update verify fails unless the staging release is signed with the release keys (well documented in bug 997732 and 1401572). We don't want to do this anymore, both because it's bad practice and it makes it more difficult to do staging releases on Try. Nick had some ideas on how to fix this in https://bugzilla.mozilla.org/show_bug.cgi?id=1401572#c1. To fix it immediately, we're going to do a binary search and replace on the updater binary from old releases to replace the release certs they have embedded in them with dep certs. I've been working on this in https://github.com/mozilla/build-tools/compare/master...mozbhearsum:cr2?expand=1 and https://github.com/mozbhearsum/gecko-unified/compare/dep-updater?expand=1. For a more long term, and less hacky fix, we're going to start building a separate updater binary with releases that always embeds the dep certs. Once it's available for all releases ahead of the current watershed, we can start using it for update verify instead of the hacking the release updater. I've been working on this part in https://github.com/mozbhearsum/gecko-unified/compare/separate-updater?expand=1.
This patch gets us building an updater binary that always embeds the dep certificates (instead of release or nightly). It gets installed to its own directory, and a subsequent patch will get us creating a test package that includes it.
This patch creates a test package for the new dep updater from https://phabricator.services.mozilla.com/D5539.
Comment on attachment 9008074 [details] bug 1490119: Add build system bits for building dep updater. Robert Strong [:rstrong] (use needinfo to contact me) has approved the revision.
Attachment #9008074 - Flags: review+
Pushed by bhearsum@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/59f5c546faa2 Add build system bits for building dep updater. r=rstrong
Keywords: leave-open
Backout by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0ce61c4a2125 Backed out changeset 59f5c546faa2 for artifact build failures. CLOSED TREE
Attachment #9008077 - Attachment is obsolete: true
Attached patch support mar cert replacement in update verify (obsolete) (deleted) — — Splinter Review
This patch adds support for mar cert replacement to the UpdateVerifyConfig class (which we use when we chunk the full update verify config) as well as to update verify itself. The most interesting parts are: - The replacement script itself, which does a binary replacement (as you suggested!), and assumes that the old and new certs will be the same length. I tried doing padding in this script, but it did not work out - the updater ended up not recognizing the certs. Tom changed our existing dep certs to be the right length by changing the subject with https://github.com/google/der-ascii. (Thankfully, nightly and release certs are exactly the same length already!) - The verify.sh changes. I took a very simple approach here and just made it possible to swap in any set of certs by looking for both other sets and trying to replace them. We'll never find nightly and release in the same updater binary (I hope), but this avoids the need for the caller to have knowledge of which certs the updater binary already has. This was tested on try in pushes like https://treeherder.mozilla.org/#/jobs?repo=try&revision=633b257b78953216e1e630fdfda1b8b04377edf1, as well as on maple in pushes like https://treeherder.mozilla.org/#/jobs?repo=maple&revision=68810f430192e2ae388a1e722b29c9e2c34da6b4. The original set of jobs on maple failed because I forgot to update the tools repo it was using -- the reruns from each platform continued to fail because of channel differences. I believe this is something that Tom is planning to address as part of his try staging work. There's an accompanying in-tree change coming via Phabricator.
Attachment #9008529 - Flags: review?(nthomas)
Imports the changes to the UpdateVerifyConfig class, and sets --override-certs for staging releases.
Comment on attachment 9008529 [details] [diff] [review] support mar cert replacement in update verify >diff --git a/release/mar_certs/dep1.der b/release/mar_certs/dep1.der >new file mode 100644 Are we ever going to need to refresh these keys ? If so, some documentation on where they come from and how to pad would be valuable. >+++ b/release/replace-updater-certs.py >+for find_cert, replace_cert in zip(*[iter(cert_pairs)]*2): Well that's quite tricksy!
Attachment #9008529 - Flags: review?(nthomas) → review+
Comment on attachment 9008536 [details] Set override_certs in update verify config creator. Nick Thomas [:nthomas] (UTC+12) has approved the revision.
Attachment #9008536 - Flags: review+
(In reply to Nick Thomas [:nthomas] (UTC+12) from comment #8) > Comment on attachment 9008529 [details] [diff] [review] > support mar cert replacement in update verify > > >diff --git a/release/mar_certs/dep1.der b/release/mar_certs/dep1.der > >new file mode 100644 > > Are we ever going to need to refresh these keys ? If so, some documentation > on where they come from and how to pad would be valuable. Good idea. I'll add a README to the directory. I know we have plans to rotate mar keys on a regular basis, so it may become important! > >+++ b/release/replace-updater-certs.py > >+for find_cert, replace_cert in zip(*[iter(cert_pairs)]*2): > > Well that's quite tricksy! :)
Attached patch tools patch, as landed (deleted) — — Splinter Review
Here's the tools patch as landed (with a README).
Attachment #9008529 - Attachment is obsolete: true
Attachment #9008704 - Flags: checked-in+
Pushed by bhearsum@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/077a6422ec4a Set override_certs in update verify config creator. r=nthomas
This patch gets us building an updater binary that always embeds the dep certificates (instead of release or nightly), and builds a new tests package that includes it. I had a lot of trouble getting the test package generated correctly due to the fact that things in "_tests" won't be included for test packages that aren't "common". My fix for that isn't ideal - I'm open to something better.
Comment on attachment 9009247 [details] bug 1490119: Add build system bits for building dep updater. Robert Strong [:rstrong] (use needinfo to contact me) has approved the revision.
Attachment #9009247 - Flags: review+
Comment on attachment 9009247 [details] bug 1490119: Add build system bits for building dep updater. Michael Shal [:mshal] has approved the revision.
Attachment #9009247 - Flags: review+
Pushed by bhearsum@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/96059b4f6d8a Add build system bits for building dep updater. r=firefox-build-system-reviewers,mshal,rstrong
Pushed by bhearsum@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1f4d6c868248 Bump timeout for automationrelevance. r=tomprince
Attached file Bump timeout for automationrelevance. (deleted) —
Bump the timeout for pulling the automationrelevance json. For very large merges, this can take awhile. For example, https://hg.mozilla.org/projects/birch/json-automationrelevance/7ecef4cf1884534b0ba6271c9f94902b3ed644a6 takes ~22 seconds.
Comment on attachment 9010411 [details] Bump timeout for automationrelevance. Tom Prince [:tomprince] has approved the revision.
Attachment #9010411 - Flags: review+
OK, so we're now at a state where staging releases will work fine when dep signed, because we do cert replacement as part of update verify. This is still only intended to be a short term fix until we watershed at a version that includes the new dep updater -- then we can download that instead of doing cert replacement in the real updater. To do that, I believe we'll need to: - Start beetmoving the dep updater into the candidates directory - Adjust the update verify config generator to point at it for the updater package (only for staging) - Ensure update verify can download/unpack/use it correctly It's worth pointing out that using the dep updater means that we won't use localized updater binaries to apply updates -- but I think that's an OK trade-off to make for staging (the alternative is to continue using the more fragile cert replacement indefinitely). If anybody thinks this is a bad idea, I'm open to reconsidering it.
chmanchester, can you help me sort out the OS X artifact build issues with https://phabricator.services.mozilla.com/D5900?
Flags: needinfo?(cmanchester)
Flags: needinfo?(bhearsum)
This patch gets us building an updater binary that always embeds the dep certificates (instead of release or nightly), and builds a new tests package that includes it. This was originally D5900, but that was backed out due to busting artifact builds. I've fixed that by removing the Makefile that Ted pointed out is unnecessary.
(In reply to Ben Hearsum (:bhearsum) from comment #23) > chmanchester, can you help me sort out the OS X artifact build issues with > https://phabricator.services.mozilla.com/D5900? Ted actually got me unblocked today.
Flags: needinfo?(cmanchester)
Comment on attachment 9010718 [details] bug 1490119: Build separate updater that always embeds dep certificates. Ted Mielczarek [:ted] [:ted.mielczarek] has approved the revision.
Attachment #9010718 - Flags: review+
Pushed by bhearsum@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eeb4c8b90872 Build separate updater that always embeds dep certificates. r=ted,firefox-build-system-reviewers
After running some tests in one of the Docker containers our tests run in, I realized we need more libraries in this package - namely, NSS and friends. My original testing was done on my laptop, and system NSS libraries made me miss this requirement :(.
Beetmoves the new dep updater into the candidates directory, like the other test packages.
This patch changes the update verify config creator to use the updater-dep package instead of the previous full build. Depending what happens with https://phabricator.services.mozilla.com/D6698 we may end needing both =\.
I'm going to be away for a number of months after this week, so I wanted to drop a quick status update. As things stand now, cert replacement is on mozilla-central and fully working -- this means that staging releases should be able to run update verify correctly without doing release signing. What remains in this bug is finishing up the work on the dep updater. It's now being created, but there's some follow-ups before we can use it: * Beetmove it to the candidates dir (https://github.com/mozilla-releng/beetmoverscript/pull/186, https://bugzilla.mozilla.org/attachment.cgi?id=9012671) * Add the other required shared libraries to the package (https://bugzilla.mozilla.org/attachment.cgi?id=9011585) -- which may or may not happen * Finish up the update verify config creator work to point at the updater-dep package (https://bugzilla.mozilla.org/attachment.cgi?id=9012691). If we don't add the required shared libraries to that package, we'll need to add both the updater-dep and full package to the configs. * Tidy up and test the update verify changes needed (https://github.com/mozilla/build-tools/compare/master...mozbhearsum:update-verify-dep-updater?expand=1). This will also need a tweak if we end up not adding other shared libraries to the updater-dep package.
Assignee: bhearsum → nobody
If someone else is going to pick up this work before bhearsum gets back, I had a comment on one of the patches in the series: https://phabricator.services.mozilla.com/D6698#193003
(In reply to bhearsum@mozilla.com (back in 2019Q1) from comment #34) > What remains in this bug is finishing up the work on the dep updater. It's > now being created, but there's some follow-ups before we can use it: > * Beetmove it to the candidates dir > (https://github.com/mozilla-releng/beetmoverscript/pull/186, Beetmoverscript bump to 7.10.1 to encompass the updater in its templates lands today.

Tom, it appears you got update verify working on try without the dep updater work. Do you think this part is still needed?

Flags: needinfo?(mozilla)

The binary hacking code seems to work well enough, that I'm not sure it worth spending more time getting working. If we do, we should consider publishing the dep-updater as a toolchain (so it can be accessed via the index), rather than publishing to archive.m.o.

Flags: needinfo?(mozilla)

(In reply to bhearsum@mozilla.com (:bhearsum) from comment #21)

OK, so we're now at a state where staging releases will work fine when dep
signed, because we do cert replacement as part of update verify. This is
still only intended to be a short term fix until we watershed at a version
that includes the new dep updater -- then we can download that instead of
doing cert replacement in the real updater.

To do that, I believe we'll need to:

  • Start beetmoving the dep updater into the candidates directory
  • Adjust the update verify config generator to point at it for the updater
    package (only for staging)
  • Ensure update verify can download/unpack/use it correctly

It's worth pointing out that using the dep updater means that we won't use
localized updater binaries to apply updates -- but I think that's an OK
trade-off to make for staging (the alternative is to continue using the more
fragile cert replacement indefinitely). If anybody thinks this is a bad
idea, I'm open to reconsidering it.

Given how long the cert replacement has been working for, I'm going to call this fixed. If we want to come back and finish up with the dep updater later, we can do that.

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #9012691 - Attachment is obsolete: true
Attachment #9012671 - Attachment is obsolete: true
Attachment #9011585 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: