Last Comment Bug 770996 - partial mars broken for mac partner builds
: partial mars broken for mac partner builds
Status: RESOLVED FIXED
: qawanted
Product: Release Engineering
Classification: Other
Component: Release Automation (show other bugs)
: other
: All All
: -- normal (vote)
: ---
Assigned To: Ben Hearsum (:bhearsum)
: Ben Hearsum (:bhearsum)
:
Mentors:
: 787235 (view as bug list)
Depends on: 787863
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-04 13:48 PDT by Ben Hearsum (:bhearsum)
Modified: 2013-08-12 21:54 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
affected
+
fixed
+
affected
+
fixed
fixed
fixed


Attachments
partial backout of bug 759318 (675 bytes, patch)
2012-07-11 05:29 PDT, Ben Hearsum (:bhearsum)
bhearsum: checked‑in+
Details | Diff | Splinter Review
Force Contents/MacOS/firefox in partials (392 bytes, patch)
2012-09-04 15:46 PDT, Nick Thomas [:nthomas]
catlee: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
nthomas: checked‑in+
Details | Diff | Splinter Review
rip out mac signing from partner repack scripts (1.24 KB, patch)
2012-09-11 11:14 PDT, Ben Hearsum (:bhearsum)
nthomas: review+
bhearsum: checked‑in+
Details | Diff | Splinter Review

Description Ben Hearsum (:bhearsum) 2012-07-04 13:48:40 PDT
Because we're resigning mac partner repacks, codesign modifies the Contents/MacOS/firefox binary. Because of this, when they attempt to apply a partial update it will fail, and they will fallback to the complete. We purposely includeI don't know how we fix this without excluding distribution/, or creating special MARs for partner rd distribution/ in the signature because otherwise people could make any changes they want through addons, etc. and they signature would still be valid and claim to be from Mozilla. I don't know any good fix right now. We could:
- Create special MARs for partner repacks (expensive, painful, no scripts to do it at this time)
- Exclude distribution from the signature (lets people alter behaviour without invalidating the signature)
- Block partial updates for partner repacks (requires a bunch of custom snippet work)
- Do nothing (bad UX)
Comment 1 Ben Hearsum (:bhearsum) 2012-07-04 13:53:49 PDT
Let's try this again:
Because we're resigning mac partner repacks, codesign modifies the Contents/MacOS/firefox binary. Because of this, when they attempt to apply a partial update it will fail, and they will fallback to the complete. We purposely included distribution/ in the signature because otherwise people could make any changes they want through addons, etc. and the signature would still be valid and claim to be from Mozilla. I don't know any good fix right now. We could:
- Create special MARs for partner repacks (expensive, painful, no scripts to do it at this time)
- Exclude distribution from the signature (lets people alter behaviour without invalidating the signature)
- Block partial updates for partner repacks (requires a bunch of custom snippet work)
- Do nothing (bad UX)
Comment 2 Ben Hearsum (:bhearsum) 2012-07-04 13:54:05 PDT
Original bug is https://bugzilla.mozilla.org/show_bug.cgi?id=759318
Comment 3 Alex Keybl [:akeybl] 2012-07-05 10:25:49 PDT
Would it make any sense to change when we sign the app (post re-pack)?
Comment 4 Ben Hearsum (:bhearsum) 2012-07-05 10:30:20 PDT
No, the problem is that there's a signature embedded within the 'firefox' binary (Contents/MacOS/firefox). That signature differs based on the contents of CodeResources (as best we can tell). Because distribution/ is part of the signature, CodeResources ends up with checksums in it - which will be different for every locale+partner/vanilla+platform combination.
Comment 5 Nick Thomas [:nthomas] 2012-07-05 16:38:44 PDT
Another issue is that the CodeResources file will (presumably) differ between vanilla and repacked builds, once bug 759328 comes into effect and distribution/* are included in it.
Comment 6 Ben Hearsum (:bhearsum) 2012-07-09 05:54:35 PDT
So, before go deep into OS X signing voodoo, is backing out bug 759318 a solution we should be considering?
Comment 7 Ben Hearsum (:bhearsum) 2012-07-09 07:22:06 PDT
Having bug 770995 would make it easier to have custom partials for these builds, but that bug is still in fantasy land with little chance of happening anytime soon.
Comment 8 Alex Keybl [:akeybl] 2012-07-10 14:49:12 PDT
(In reply to Ben Hearsum [:bhearsum] from comment #6)
> So, before go deep into OS X signing voodoo, is backing out bug 759318 a
> solution we should be considering?

We took bug 759318 mostly for correctness, since code signing was an install-driven feature as opposed to a security-driven feature. Backing out sounds like a good plan.
Comment 9 Ben Hearsum (:bhearsum) 2012-07-10 14:52:43 PDT
(In reply to Alex Keybl [:akeybl] from comment #8)
> (In reply to Ben Hearsum [:bhearsum] from comment #6)
> > So, before go deep into OS X signing voodoo, is backing out bug 759318 a
> > solution we should be considering?
> 
> We took bug 759318 mostly for correctness, since code signing was an
> install-driven feature as opposed to a security-driven feature. Backing out
> sounds like a good plan.

OK, great. I'll make this happen tomorrow.
Comment 10 Ben Hearsum (:bhearsum) 2012-07-11 05:29:25 PDT
Created attachment 641017 [details] [diff] [review]
partial backout of bug 759318

Landed this on beta, aurora, and central:
https://hg.mozilla.org/releases/mozilla-beta/rev/c92edb5a0ac2
https://hg.mozilla.org/releases/mozilla-aurora/rev/226a2d776e84
https://hg.mozilla.org/mozilla-central/rev/faef72257478

Should be good to go here with 14.0 now.
Comment 11 Ben Hearsum (:bhearsum) 2012-08-31 14:21:18 PDT
*** Bug 774618 has been marked as a duplicate of this bug. ***
Comment 12 Ben Hearsum (:bhearsum) 2012-08-31 14:23:27 PDT
I'm not sure this is fixed, as it turns out. While investigating bug 787235 I did a comparison of a vanilla en-US 15.0 vs. a partner and found this:
bld-lion-r5-010:vanilla cltbld$ md5 Firefox.app/Contents/MacOS/firefox
MD5 (Firefox.app/Contents/MacOS/firefox) = 91e12442bcb3dadbcd37cfd89eb8f907

bld-lion-r5-010:partner cltbld$ md5 Firefox.app/Contents/MacOS/firefox
MD5 (Firefox.app/Contents/MacOS/firefox) = 9f03b204f94a4bc56f1a66998cba468e


Which means that partner builds of 15.0 that get a partial update to 16.0 will fail, and they won't fall back due to bug 774618.
Comment 13 Ben Hearsum (:bhearsum) 2012-08-31 14:24:56 PDT
*** Bug 787235 has been marked as a duplicate of this bug. ***
Comment 14 Alex Keybl [:akeybl] 2012-09-02 15:03:47 PDT
(In reply to Ben Hearsum [:bhearsum] from comment #12)
> Which means that partner builds of 15.0 that get a partial update to 16.0
> will fail, and they won't fall back due to bug 774618.

Adding qawanted to confirm what the experience is when updating a FF14.0.1 partner build to FF15.0 (presumably bug 774618).

This seems pretty concerning - do you agree that our only option for updating FF14/15 partner build users may be to create custom partial updates to FF16? Then we'd presumably stop the bleeding in the future by fixing bug 774618 in FF16?
Comment 15 Nick Thomas [:nthomas] 2012-09-02 20:28:16 PDT
Urgh. I can confirm a en-US 14.0.1 funnelcake14 build fails the partial (this bug) then doesn't failover to the complete (bug 774618). It will affect about 90k ADI.

We can block 14.0.1 -> 15.0 partials for mac to work around this in the meantime. Bug 787863 for that.
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-04 11:46:40 PDT
What, if any, are the implications here that we should be prepared for in the case of a 15.0.1 chemspill?
Comment 17 Johnathan Nightingale [:johnath] 2012-09-04 14:23:58 PDT
(adding rstrong in case there are client side update issues, as opposed to just mar generation)
Comment 18 Chris AtLee [:catlee] 2012-09-04 14:47:26 PDT
AIUI, mac signing will always modify the firefox binary with new signature information

nthomas suggested we always include the full firefox binary in the partial mar.

we can also look at disabling signing for partner repacks.
Comment 19 Nick Thomas [:nthomas] 2012-09-04 15:10:43 PDT
(In reply to Chris AtLee [:catlee] from comment #18)
> nthomas suggested we always include the full firefox binary in the partial
> mar.

This would be the Contents/MacOS/firefox executable, which is ~55KB and would have negligible impact on the partial mar size. The prefix should make it mac-specific.

IIRC we'd have to modify this line
 http://hg.mozilla.org/releases/mozilla-release/file/default/tools/update-packaging/make_incremental_update.sh#l60
to do that, as we don't currently pass any forces in from buildbot.

AFAIK this wouldn't make the signing any less secure than attachment 641017 [details] [diff] [review] intended to leave us, ie we're not enforcing that the contents of distribution/ be unmodified either way, but I wouldn't claim to be well versed in this security realm.
Comment 20 Nick Thomas [:nthomas] 2012-09-04 15:46:08 PDT
Created attachment 658280 [details] [diff] [review]
Force Contents/MacOS/firefox in partials

Using this I recreated the 13.0.1 -> 15.0 partial mar for vanilla mac en-US, then unpacked the original mar to before/ and the new one to after/. 

$ diff -ru before/ after/ | cut -c-80 
Only in before/Contents/MacOS: firefox.patch
Only in after/Contents/MacOS: firefox
diff -ru before/update.manifest after/update.manifest
--- before/update.manifest	2012-09-04 15:39:12.000000000 -0700
+++ after/update.manifest	2012-09-04 15:37:12.000000000 -0700
@@ -36,7 +36,7 @@
 patch "Contents/MacOS/libfreebl3.dylib.patch" "Contents/MacOS/libfreebl3.dylib"
 add "Contents/MacOS/libfreebl3.chk"
 patch "Contents/MacOS/firefox-bin.patch" "Contents/MacOS/firefox-bin"
-patch "Contents/MacOS/firefox.patch" "Contents/MacOS/firefox"
+add "Contents/MacOS/firefox"
 patch-if "Contents/MacOS/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}" "Co
 patch-if "Contents/MacOS/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}" "Co
 patch "Contents/MacOS/dictionaries/en-US.aff.patch" "Contents/MacOS/dictionarie
diff -ru before/updatev2.manifest after/updatev2.manifest
--- before/updatev2.manifest	2012-09-04 15:39:13.000000000 -0700
+++ after/updatev2.manifest	2012-09-04 15:37:13.000000000 -0700
@@ -37,7 +37,7 @@
 patch "Contents/MacOS/libfreebl3.dylib.patch" "Contents/MacOS/libfreebl3.dylib"
 add "Contents/MacOS/libfreebl3.chk"
 patch "Contents/MacOS/firefox-bin.patch" "Contents/MacOS/firefox-bin"
-patch "Contents/MacOS/firefox.patch" "Contents/MacOS/firefox"
+add "Contents/MacOS/firefox"
 patch-if "Contents/MacOS/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}" "Co
 patch-if "Contents/MacOS/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}" "Co
 patch "Contents/MacOS/dictionaries/en-US.aff.patch" "Contents/MacOS/dictionarie

ie include the whole firefox file, and overwrite instead of patching in the manifests. Otherwise no change.

The file size increased 10882 bytes to 17776750 bytes.
Comment 21 Nick Thomas [:nthomas] 2012-09-04 16:42:15 PDT
Comment on attachment 658280 [details] [diff] [review]
Force Contents/MacOS/firefox in partials

The test mar I created successfully updated 13.0.1 funnelcake11 to 15.0. I think this is fairly safe but there may be some risk to put this straight into a chemspill. What do you think akeybl ?
Comment 22 Alex Keybl [:akeybl] 2012-09-04 16:54:28 PDT
(In reply to Nick Thomas [:nthomas] from comment #21)
> Comment on attachment 658280 [details] [diff] [review]
> Force Contents/MacOS/firefox in partials
> 
> The test mar I created successfully updated 13.0.1 funnelcake11 to 15.0. I
> think this is fairly safe but there may be some risk to put this straight
> into a chemspill. What do you think akeybl ?

Is the alternative to provide complete mars for all partner build updates? Basically, I'd like to take the lowest risk solution that successfully moves our users to the latest version.
Comment 23 Nick Thomas [:nthomas] 2012-09-04 17:11:01 PDT
Lower touch than that, but still fairly involved. We'd need to 
* enumerate the list of all the active partner channels (eg release-cck-yandex, release-cck-mozilla14 for funnelcake14)
* figure out the versions where we have signing on mac, and also offer a partial upgrade
* create empty snippets each pair of channel & version, which block falling back to the partial on the release channel (for all locales etc)
* test and publish

As new releases come along we'd need to add new empty snippets, so there's an ongoing cost. It would also slow down pushing new snippets out as we accumulate more snippets to backup/push.

In comparison changing the partial generation is a once-off change, albeit one with a larger scope than just the partners. From inspection of the mar generation code the patch should not put linux or windows at risk because they don't share the Contents/MacOS prefix.
Comment 24 Chris AtLee [:catlee] 2012-09-05 05:41:32 PDT
Comment on attachment 658280 [details] [diff] [review]
Force Contents/MacOS/firefox in partials

Review of attachment 658280 [details] [diff] [review]:
-----------------------------------------------------------------

this looks safe to me
Comment 25 Nick Thomas [:nthomas] 2012-09-06 16:59:51 PDT
Comment on attachment 658280 [details] [diff] [review]
Force Contents/MacOS/firefox in partials

http://hg.mozilla.org/integration/mozilla-inbound/rev/8943f92197c3
Comment 26 Ed Morley [:emorley] 2012-09-07 08:47:18 PDT
https://hg.mozilla.org/mozilla-central/rev/8943f92197c3
Comment 27 Chris More [:cmore] 2012-09-10 09:00:07 PDT
My funnelcake 14 that was unable to update automatically updated to 15.0.1 after I restarted the browser today. Last week, this was not the case and the update would not apply.
Comment 28 Ben Hearsum (:bhearsum) 2012-09-10 09:02:08 PDT
(In reply to Chris More [:cmore] from comment #27)
> My funnelcake 14 that was unable to update automatically updated to 15.0.1
> after I restarted the browser today. Last week, this was not the case and
> the update would not apply.

The work that was done in this bug (in particular, attachment #658280 [details] [diff] [review]) fixed that for 15.0.1. Thanks for confirming!
Comment 29 Nick Thomas [:nthomas] 2012-09-10 19:31:24 PDT
Comment on attachment 658280 [details] [diff] [review]
Force Contents/MacOS/firefox in partials

This is working as expected in Nightly builds, eg from the last-update.log:

EXECUTE ADD Contents/MacOS/firefox
...
FINISH ADD Contents/MacOS/firefox
...
succeeded
calling QuitProgressUI


[Approval Request Comment]
Bug caused by (feature/regressing bug #): mac signing in general (bug 730924) and bug 759318 (sign partner builds harder to avoid malware)
User impact if declined: 13.0 through 15.0.1 mac partner users will have a bad update experience
Testing completed (on m-c, etc.): baked on Nightly for 3 updates
Risk to taking this patch (and alternatives if risky):  low risk, this merely forces an update for a file we would have patched
String or UUID changes made by this patch: none
Comment 30 Ben Hearsum (:bhearsum) 2012-09-11 11:14:45 PDT
Created attachment 660161 [details] [diff] [review]
rip out mac signing from partner repack scripts

Nick, here's the other approach you and I talked about a bit in e-mail, and it was also brought up in the 15.0 post mortem today and nobody had objections to it.

To check that this won't have negative side-effects I diff'ed a partner build vs. its vanilla one, and found the only differences to be the existence of distribution/, and the firefox binary itself. The distribution/ difference is expected and doesn't break the signature, and with this patch the firefox binary will end up the same again, fixing the partials and removing the need to force a full update for the firefox binary.
Comment 31 Chris More [:cmore] 2012-09-11 11:22:04 PDT
 
> The work that was done in this bug (in particular, attachment #658280 [details] [diff] [review]
>  fixed that for 15.0.1. Thanks for confirming!

Just to clarify, we shouldn't be concerned about people being stuck on fx 14 mac partner builds (without having to manually upgrade) going forward after they restart their browser?
Comment 32 Ben Hearsum (:bhearsum) 2012-09-11 12:11:57 PDT
(In reply to Chris More [:cmore] from comment #31)
>  
> > The work that was done in this bug (in particular, attachment #658280 [details] [diff] [review] [diff] [review]
> >  fixed that for 15.0.1. Thanks for confirming!
> 
> Just to clarify, we shouldn't be concerned about people being stuck on fx 14
> mac partner builds (without having to manually upgrade) going forward after
> they restart their browser?

Right now, partials for partner users on Mac are failing due to this bug. I would expect the fallback to a complete not to happen either due to bug 774618, but your experience shows this isn't always the case. This bug will be fixed for the next release though, so we don't have to worry about people being stranded due to this at that point.
Comment 33 Alex Keybl [:akeybl] 2012-09-11 13:42:27 PDT
Comment on attachment 658280 [details] [diff] [review]
Force Contents/MacOS/firefox in partials

[Triage Comment]
Approving in support of working updates for partner repacks in future releases.
Comment 34 Nick Thomas [:nthomas] 2012-09-11 13:55:21 PDT
(In reply to Ben Hearsum [:bhearsum] from comment #32)
> (In reply to Chris More [:cmore] from comment #31)
> >  
> > > The work that was done in this bug (in particular, attachment #658280 [details] [diff] [review] [diff] [review] [diff] [review]
> > >  fixed that for 15.0.1. Thanks for confirming!

We haven't actually built any release mar files with this patch yet, instead we've suppressed partial updates for mac partner builds (in the update server). The remaining complete updates replace the firefox executable rather than patching so the update works ok, but at the cost of a much larger download.

> > Just to clarify, we shouldn't be concerned about people being stuck on fx 14
> > mac partner builds (without having to manually upgrade) going forward after
> > they restart their browser?
> 
> Right now, partials for partner users on Mac are failing due to this bug. I
> would expect the fallback to a complete not to happen either due to bug
> 774618, but your experience shows this isn't always the case. This bug will
> be fixed for the next release though, so we don't have to worry about people
> being stranded due to this at that point.

I think Chris got a complete update, about 32MB instead of 14MB right ? You could check by looking at Firefox.app/Contents/MacOS/updates.xml. 

At any rate for 16.0 we'll probably have a complete from 13.0 (when signing started), and partials that force firefox for anything more recent than that. Plus we'll stop re-signing mac partner builds then, so the underlying issue will go away.
Comment 35 Chris More [:cmore] 2012-09-11 14:02:33 PDT
Here is my updates.xml:

<updates xmlns="http://www.mozilla.org/2005/app-update"><update appVersion="undefined" buildID="undefined" channel="default" displayVersion="undefined" extensionVersion="undefined" installDate="undefined" isCompleteUpdate="false" name="undefined" serviceURL="undefined" showNeverForVersion="false" showPrompt="false" showSurvey="false" type="undefined" version="undefined" detailsURL="http://www.mozilla.com/en-US/firefox/releases/" statusText="The Update was successfully installed"/></updates>
Comment 36 Nick Thomas [:nthomas] 2012-09-11 14:14:45 PDT
Comment on attachment 658280 [details] [diff] [review]
Force Contents/MacOS/firefox in partials

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ad864f65eb7
https://hg.mozilla.org/releases/mozilla-beta/rev/b4dd8cdec0ad

akeybl, I'd like to make sure this patch is included in any 15.0.2 chemspill, but understand there is no certainly about if we are doing one. Should I go ahead and request approval-mozilla-release so that we don't miss it in a rush ?
Comment 37 Ben Hearsum (:bhearsum) 2012-09-11 14:15:46 PDT
Comment on attachment 660161 [details] [diff] [review]
rip out mac signing from partner repack scripts

With this landed, any future releases we do will have a consistent 'firefox' binary between vanilla and partner repacks.
Comment 38 Ben Hearsum (:bhearsum) 2012-10-26 13:14:46 PDT
Modulo bug 805788, which tracks _another_ addition to files excluded from the signature, we're all done here AFAICT.

Note You need to log in before you can comment on or make changes to this bug.