1490119 - make update verify work in staging with dep signing

Status: RESOLVED FIXED

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

Description

[[e-mail] (:bhearsum)]

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.

Comment 1

[[e-mail] (:bhearsum)]

Attachment: bug 1490119: Add build system bits for building dep updater.

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.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: bug 1490119: Try to put the dep updater into its own package.

This patch creates a test package for the new dep updater from https://phabricator.services.mozilla.com/D5539.

Comment 3

[Robert Strong (they/them - no direct email)]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59f5c546faa2
Add build system bits for building dep updater. r=rstrong

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ce61c4a2125
Backed out changeset 59f5c546faa2 for artifact build failures. CLOSED TREE

Comment 6

[[e-mail] (:bhearsum)]

Attachment: support mar cert replacement in update verify

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.

Comment 7

[[e-mail] (:bhearsum)]

Attachment: Set override_certs in update verify config creator.

Imports the changes to the UpdateVerifyConfig class, and sets --override-certs for staging releases.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

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!

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 9008536 [details]
Set override_certs in update verify config creator.

Nick Thomas [:nthomas] (UTC+12) has approved the revision.

Comment 10

[[e-mail] (:bhearsum)]

(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!

:)

Comment 11

[[e-mail] (:bhearsum)]

Attachment: tools patch, as landed

Here's the tools patch as landed (with a README).

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/077a6422ec4a
Set override_certs in update verify config creator. r=nthomas

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/077a6422ec4a

Comment 14

[[e-mail] (:bhearsum)]

Attachment: bug 1490119: Add build system bits for building dep updater.

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 15

[Robert Strong (they/them - no direct email)]

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.

Comment 16

[Michael Shal [:mshal]]

Comment on attachment 9009247 [details]
bug 1490119: Add build system bits for building dep updater.

Michael Shal [:mshal] has approved the revision.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96059b4f6d8a
Add build system bits for building dep updater. r=firefox-build-system-reviewers,mshal,rstrong

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f4d6c868248
Bump timeout for automationrelevance. r=tomprince

Comment 19

[[e-mail] (:bhearsum)]

Attachment: Bump timeout for automationrelevance.

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 20

[Tom Prince [:tomprince]]

Comment on attachment 9010411 [details]
Bump timeout for automationrelevance.

Tom Prince [:tomprince] has approved the revision.

Comment 21

[[e-mail] (:bhearsum)]

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.

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/96059b4f6d8a

Comment 23

[[e-mail] (:bhearsum)]

chmanchester, can you help me sort out the OS X artifact build issues with https://phabricator.services.mozilla.com/D5900?

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 96059b4f6d8a (Bug 1490119) for breaking OSX builds.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=59f5c546faa289cd0b7b713dc5ac9e4e53cb0c02

Backout link: https://hg.mozilla.org/mozilla-central/rev/ac9f1219d11bf1a56ec1ace8e3ba9ff113b5cacb

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1f4d6c868248

Comment 26

[[e-mail] (:bhearsum)]

Attachment: bug 1490119: Build separate updater that always embeds dep certificates.

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.

Comment 27

[[e-mail] (:bhearsum)]

(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.

Comment 28

[(not currently active) Ted Mielczarek]

Comment on attachment 9010718 [details]
bug 1490119: Build separate updater that always embeds dep certificates.

Ted Mielczarek [:ted] [:ted.mielczarek] has approved the revision.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eeb4c8b90872
Build separate updater that always embeds dep certificates. r=ted,firefox-build-system-reviewers

Comment 30

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/eeb4c8b90872

Comment 31

[[e-mail] (:bhearsum)]

Attachment: bug 1490119: Add more required shared libraries to updater-dep package.

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 :(.

Comment 32

[[e-mail] (:bhearsum)]

Attachment: Beetmove the dep updater into the candidates directory.

Beetmoves the new dep updater into the candidates directory, like the other test packages.

Comment 33

[[e-mail] (:bhearsum)]

Attachment: WIP: First pass on update verify config creator for dep updater.

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 =\.

Comment 34

[[e-mail] (:bhearsum)]

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.

Comment 35

[(not currently active) Ted Mielczarek]

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

Comment 36

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/5ccec2d040a6d401261013979140b502b9dd4eff

Comment 37

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to [e-mail] (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.

Comment 38

[Tom Prince [:tomprince]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f01536d9b9a7
https://hg.mozilla.org/releases/mozilla-esr60/rev/ff621ab0c6f6

Comment 39

[[e-mail] (:bhearsum)]

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

Comment 40

[Tom Prince [:tomprince]]

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.

Comment 41

[[e-mail] (:bhearsum)]

(In reply to [e-mail] (: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.