1047584 - Modify file structure of Firefox.app to allow for OSX v2 signing

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Stephen A Pohl [:spohl]]

For OSX v2 signatures, we need to modify the structure of our .app bundle. In particular, we need to move all non-code files out of Contents/MacOS and into Contents/Resources. This applies recursively to any .app bundles packaged by the top-level .app bundle as well (crashreporter.app, updater.app etc.). Also, any directory under Contents/MacOS will be interpreted as bundle. Since we have many directories there that aren't bundles (like browser, defaults etc.) we need to move those to Resources as well.

Note: although we could sign individual non-code files under Contents/MacOS, these would create xattrs that are not straightforward to package in MARs and deploy during an update. There is no alternative to moving any non-bundle directory out of Contents/MacOS however.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: changes to Makefile.in (wip)

This simply moves the file into the right place before packaging. We'll have to figure out the proper way of doing this before we can land, but it should unblock the work that's needed in 'make package'.

Comment 2

[Stephen A Pohl [:spohl]]

Attachment: Patch

This moves the files into their new place and updates package-manifest for the new structure:
1. Code files (executable code, dylibs, .app bundles) remain in *.app/Contents/MacOS
2. Data files that don't change at runtime are moved to *.app/Contents/Resources
3. Data files that could change at runtime now live under *.app/MozResources

I will request feedback/reviews once we've fixed up all the dependencies that broke due to this change.

Comment 3

[Stephen A Pohl [:spohl]]

Let's make sure to delete the in-tree CodeResources file in the final patch, since it's no longer used for signing.

Comment 4

[Stephen A Pohl [:spohl]]

These are the current proposed changes:

New/added directories:
> .app/MozResources/


Moved from .app/Contents/MacOS to .app/Contents/Resources:
> application.ini
> browser/
> dependentlibs.list
> dictionaries/
> libfreebl3.chk
> libnssdbm3.chk
> libsoftokn3.chk
> omni.ja
> platform.ini
> removed-files
> update-settings.ini
> update.ini
> webapprt/

Moved from .app/Contents/MacOS/crashreporter.app/Contents/MacOS to .app/Contents/MacOS/crashreporter.app/Contents/Resources:
> crashreporter.ini

Moved from .app/Contents/MacOS/res to .app/Contents/Resources:
> MainMenu.nib/
> cursors/

Moved from .app/Contents/MacOS to .app/MozResources:
> defaults/

Comment 5

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

(In reply to Stephen Pohl [:spohl] from comment #4)
> Moved from .app/Contents/MacOS to .app/Contents/Resources:
> > libfreebl3.chk
> > libnssdbm3.chk
> > libsoftokn3.chk

Moving these away from the matching dylibs may break FIPS mode, I suggest talking to someone from the NSS team (eg :kaie).

Comment 6

[(not currently active) Ted Mielczarek]

We talked about this and I said that was acceptable to get this working.

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Patch

1. Changed the name of the .lproj directory under Resources to en-US.lproj, from en.lproj previously. This avoids having to hardcode "en" in the packaging manifest, or trim "-US" from "en-US". I couldn't find a reason to justify the additional work to do so.
2. Removed CodeResources files since they are no longer used in v2 signing.
3. Fixed an issue where document.icns, firefox.icns and the .lproj directory didn't get packaged into Resources.
4. Changed CFBundleIconFile to "firefox.icns" instead of just "firefox" in the Info.plist. Without this, the bundle would show a default icon instead of the firefox icon. Although the Apple docs say that the file extension is optional, OSX seems to get confused by the other resources and can't find the proper icon when the extension isn't specified.

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Patch

Forgot to remove CodeResources from package-manifest.

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: Patch

...and packager.mk. Will launch another try build once the trees reopen, but bhearsum was able to confirm that the build gets signed correctly.

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Patch

Added minor tweak to createprecomplete.py to generate the precomplete file correctly.

Benjamin, we still need to run this through all the tests and fix things up, but before I'm going further down this path I wanted to check with you if this approach is acceptable to you.

To have a working .app bundle that launches and runs, the following patches need to be applied:

1. This patch (bug 1047584).
2. Patch from bug 1048687.
3. Patch from bug 1050944.

I'll ask for your feedback in bug 1048687 and bug 1050944 as well.

I'll also upload a patch shortly that enables signing of this new structure on our build machines.

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: Enable v2 signing

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 8473006 [details] [diff] [review]
Enable v2 signing

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

This is right, just make sure it lands at the same time as the other changes, of course.

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Patch

Renaming the .lproj directory to en-US.lproj would break a bunch of l10n scripts, so un-renaming to en.lproj.

Comment 14

[Benjamin Smedberg]

Comment on attachment 8473682 [details] [diff] [review]
Patch

In browser/app/Makefile.in the variables APPRESOURCES and MOZRESOURCES are unnecessary: they don't change based on configuration or make thing easier to read. Please remove them and just put "Resources" or "MozResources" directly in their place.

The repackage step needs some work. rsync-then-move kinda sucks for this, resulting in lots of extra disk I/O in the common-rebuild case. I think it would be far better to do this:

* have a list of files that go in MacOS instead of Resources
* have a list of files that go in MozResources instead of Resources

Do the initial rsync to Resources/ with --exclude-from to avoid syncing the excluded files.
Do a separate rsync to MacOS and MozResources using rsync --include-from.

Comment 15

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback. Still waiting for test harnesses to be updated and start passing before requesting review.

Comment 16

[:Gijs (he/him)]

I hate scope creep as much as the next guy... but is there any chance this is an opportunity to stop duplicating all of omni.ja between the two sides of the universal build and so to reduce the size of our updates, dmgs, and .app bundles?

Comment 17

[Stephen A Pohl [:spohl]]

(In reply to :Gijs Kruitbosch from comment #16)
> I hate scope creep as much as the next guy... but is there any chance this
> is an opportunity to stop duplicating all of omni.ja between the two sides
> of the universal build and so to reduce the size of our updates, dmgs, and
> .app bundles?

It would be great to have a bug on file to track this (please CC me). The more detail, the better. However, seeing how we're scrambling to get even the most basic things done in time for the v2 changes, I don't believe we'll be able to address this right now.

Comment 18

[Stephen A Pohl [:spohl]]

Attachment: Patch

Fixed OSX debug build failure.

Comment 19

[Stephen A Pohl [:spohl]]

Comment on attachment 8487476 [details] [diff] [review]
Patch

Try push is almost completely green, so requesting review (updater xpcshell test failures and gaia python integration test suite failures are handled in separate bugs):
https://tbpl.mozilla.org/?tree=Try&rev=7362867ad903

Comment 20

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/6faa732a5a82
https://hg.mozilla.org/projects/oak/rev/012e80a2ad45

Comment 21

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

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/3ff20d4c7f39
https://hg.mozilla.org/integration/fx-team/rev/fbc1ceea4d6f

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ff20d4c7f39
https://hg.mozilla.org/mozilla-central/rev/fbc1ceea4d6f

Comment 23

[Mike Hommey [:glandium]]

Is OSX actually happy with Firefox loading the binary browsercomps component from the resources directory (which, even if OSX does it, feels very wrong)?

Comment 24

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

It is from a signing point of view. We might change its location to something more suitable after this initial work has been uplifted.

Comment 25

[Stephen A Pohl [:spohl]]

Attachment: Patch for aurora

Approval Request Comment
[Feature/regressing bug #]: OSX v2 bundle signing
[User impact if declined]: Users would see a Gatekeeper warning saying that Firefox wasn't signed by a known developer and the user would be unable to start it without jumping through hoops.
[Describe test coverage new/current, TBPL]: Most of these changes have been on m-c for over a week. Virtually all of our tests exercise this feature by verifying that Firefox can be launched and additional, dedicated v2 signature checks were added to mozmill.
[Risks and why]: The bundle structure of our .app bundles has changed quite significantly. Although tested quite thoroughly, there may be edge-cases that were missed and that will need to be fixed before release. More exposure on aurora will help us find these cases.
[String/UUID change made/needed]: none

Aurora try run is in progress here:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=be4520a330b1

For reference, these are the patches that were folded into this combined patch:
bug1047584-enableV2Signing
bug1047584
bug1048687
bug1050944
bug1046924
bug1055774
bug1059504
bug1053838
bug1060652
bug1060562
bug1064952
bug1064910
bug1066123
bug1047738
bug1059467
bug1059567
bug1064523
bug1058182
bug1068439
bug1070661
bug1070148
bug1070149
bug1070863
bug1071134
bug1071465
bug1072554
bug1072716
bug1072722
bug1074000
bug1074044
bug1075169
bug1075492
bug1075691
bug1076370
bug1076977
bug1076941
bug1077099
bug1077099-followup
bug1077282
bug1077282-cleanupCrashreporter
bug1077282-workaround
bug1077268
bug1075981
bug1078640
bug1079520
bug1079655-1
bug1079655-2

Comment 26

[Stephen A Pohl [:spohl]]

Attachment: Patch for aurora

Updated patch with most recent patch from bug 1077268. New try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2e00d0933001

Comment 27

[Stephen A Pohl [:spohl]]

Attachment: Patch for aurora

Now also with patch from bug 1080395.

Comment 28

[Stephen A Pohl [:spohl]]

Comment on attachment 8503238 [details] [diff] [review]
Patch for aurora

Please see comment 25 for approval request comment.

Comment 29

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

Comment on attachment 8503238 [details] [diff] [review]
Patch for aurora

I believe that the patch from bug 1075169 isn't included. Still checking others

Comment 30

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

Specifically,

diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
--- a/toolkit/mozapps/update/updater/updater.cpp
+++ b/toolkit/mozapps/update/updater/updater.cpp
@@ -1986,16 +1986,24 @@ ProcessReplaceRequest()
   if (rv) {
     LOG(("Moving destDir to tmpDir failed, err: %d", rv));
     return rv;
   }
 
   LOG(("Begin moving newDir (" LOG_S ") to destDir (" LOG_S ")",
        newDir, destDir));
   rv = rename_file(newDir, destDir, true);
+#ifdef XP_MACOSX
+  if (rv) {
+    LOG(("Moving failed. Begin copying newDir (" LOG_S ") to destDir (" LOG_S ")",
+         newDir, destDir));
+    copy_recursive_skiplist<0> skiplist;
+    rv = ensure_copy_recursive(newDir, destDir, skiplist);
+  }
+#endif
   if (rv) {
     LOG(("Moving newDir to destDir failed, err: %d", rv));
     LOG(("Now, try to move tmpDir back to destDir"));
     ensure_remove_recursive(destDir);
     int rv2 = rename_file(tmpDir, destDir, true);
     if (rv2) {
       LOG(("Moving tmpDir back to destDir failed, err: %d", rv2));
     }

Comment 31

[Stephen A Pohl [:spohl]]

Attachment: Patch for aurora

This should fix it.

Comment 32

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

Comment on attachment 8503413 [details] [diff] [review]
Patch for aurora

From comment #25 - see comment #25 for more info

Approval Request Comment
[Feature/regressing bug #]: OSX v2 bundle signing
[User impact if declined]: Users would see a Gatekeeper warning saying that Firefox wasn't signed by a known developer and the user would be unable to start it without jumping through hoops.
[Describe test coverage new/current, TBPL]: Most of these changes have been on m-c for over a week. Virtually all of our tests exercise this feature by verifying that Firefox can be launched and additional, dedicated v2 signature checks were added to mozmill.
[Risks and why]: The bundle structure of our .app bundles has changed quite significantly. Although tested quite thoroughly, there may be edge-cases that were missed and that will need to be fixed before release. More exposure on aurora will help us find these cases.
[String/UUID change made/needed]: none

Comment 33

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8503413 [details] [diff] [review]
Patch for aurora

This is a substantial patch and adds risk to the 34 release. This is manageable risk and rstrong, spohl and others have been very diligent with their work to limit the impact. Furthermore, this change is needed to support the new Gatekeeper requirements that we are currently supporting with Apple's help.

cc lizzard and juanb to let them know that this change is about to hit Aurora. If possible, we should spend time before beta1 ships testing update and install on OSX 10.9.5 and older and any other platforms that rstrong and spohl think we should sanity check.

Comment 34

[Lawrence Mandel [:lmandel] (use needinfo)]

[Tracking Requested - why for this release]:

Comment 35

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

Comment on attachment 8503413 [details] [diff] [review]
Patch for aurora

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/42dba12fb601

Comment 36

[Florian Bender]

(In reply to Stephen Pohl [:spohl] from comment #17)
> (In reply to :Gijs Kruitbosch from comment #16)
> > I hate scope creep as much as the next guy... but is there any chance this
> > is an opportunity to stop duplicating all of omni.ja between the two sides
> > of the universal build and so to reduce the size of our updates, dmgs, and
> > .app bundles?
> 
> It would be great to have a bug on file to track this (please CC me). The
> more detail, the better. However, seeing how we're scrambling to get even
> the most basic things done in time for the v2 changes, I don't believe we'll
> be able to address this right now.

Did this happen? What's the bug number?

Comment 37

[:Gijs (he/him)]

(In reply to Florian Bender from comment #36)
> (In reply to Stephen Pohl [:spohl] from comment #17)
> > (In reply to :Gijs Kruitbosch from comment #16)
> > > I hate scope creep as much as the next guy... but is there any chance this
> > > is an opportunity to stop duplicating all of omni.ja between the two sides
> > > of the universal build and so to reduce the size of our updates, dmgs, and
> > > .app bundles?
> > 
> > It would be great to have a bug on file to track this (please CC me). The
> > more detail, the better. However, seeing how we're scrambling to get even
> > the most basic things done in time for the v2 changes, I don't believe we'll
> > be able to address this right now.
> 
> Did this happen? What's the bug number?

This got lost in my bugmail, and it did not happen. I don't know if it makes sense to pursue this; it's somewhat likely that it will be a difficult change to make, and I don't know if the investment would be worth it considering that 32-bit support is basically a dead end. OTOH, we sadly have quite a number of 10.6 users still, and AFAIK there are still no concrete plans as to when we'll be dropping support.

Comment 38

[Florian Bender]

Filed Bug 1103566.

Comment 39

[Mike Hommey [:glandium]]

(In reply to Nick Thomas [:nthomas] from comment #5)
> (In reply to Stephen Pohl [:spohl] from comment #4)
> > Moved from .app/Contents/MacOS to .app/Contents/Resources:
> > > libfreebl3.chk
> > > libnssdbm3.chk
> > > libsoftokn3.chk
> 
> Moving these away from the matching dylibs may break FIPS mode, I suggest
> talking to someone from the NSS team (eg :kaie).

s/may break/breaks/

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> We talked about this and I said that was acceptable to get this working.

Was a followup bug filed to make FIPS mode work with the chk files in the Resources directory?

Comment 40

[Stephen A Pohl [:spohl]]

(In reply to Mike Hommey [:glandium] from comment #39)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> > We talked about this and I said that was acceptable to get this working.
> 
> Was a followup bug filed to make FIPS mode work with the chk files in the
> Resources directory?

Bug 1100424. Also see bug 1096494 for more discussion.

Comment 41

[Lawrence Mandel [:lmandel] (use needinfo)]

Release noted as "Firefox signed by Apple OS X version 2 signature".

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Clearing the needinfo. I'm not sure if we've done any particular followup, but I also haven't heard from support or user advocacy that Mac users have had problems downloading and installing Firefox. 

We have a knowledgebase article to catch this problem: https://support.mozilla.org/en-US/kb/firefox-cant-be-opened-after-you-install-it-on-mac 

Juan is there anything further you want to do to check this out?