The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

Release Engineering
General Automation
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: spohl, Assigned: spohl)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox32 wontfix, firefox33 wontfix, firefox34+ fixed, firefox35 fixed, firefox-esr31 affected, relnote-firefox 34+)

Details

Attachments

(3 attachments, 11 obsolete attachments)

653 bytes, patch
bhearsum
: review+
Details | Diff | Splinter Review
14.63 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
868.36 KB, patch
spohl
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8466438 [details] [diff] [review]
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'.
(Assignee)

Updated

3 years ago
Depends on: 1047719
(Assignee)

Updated

3 years ago
Depends on: 1047728
(Assignee)

Updated

3 years ago
Depends on: 1047738
(Assignee)

Updated

3 years ago
Depends on: 1047740
(Assignee)

Updated

3 years ago
Depends on: 1047742
(Assignee)

Comment 2

3 years ago
Created attachment 8467523 [details] [diff] [review]
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.
Assignee: nobody → spohl.mozilla.bugs
Attachment #8466438 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1046906
(Assignee)

Updated

3 years ago
Depends on: 1048687
(Assignee)

Comment 3

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

Comment 4

3 years ago
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/
(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).
We talked about this and I said that was acceptable to get this working.
(Assignee)

Updated

3 years ago
No longer depends on: 1048687
(Assignee)

Updated

3 years ago
No longer depends on: 1047742
(Assignee)

Updated

3 years ago
No longer depends on: 1047740
(Assignee)

Updated

3 years ago
No longer depends on: 1047738
(Assignee)

Updated

3 years ago
No longer depends on: 1047728
(Assignee)

Updated

3 years ago
No longer depends on: 1047719
(Assignee)

Updated

3 years ago
Group: mozilla-employee-confidential
(Assignee)

Comment 7

3 years ago
Created attachment 8471616 [details] [diff] [review]
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.
Attachment #8467523 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1046924
(Assignee)

Comment 8

3 years ago
Created attachment 8472370 [details] [diff] [review]
Patch

Forgot to remove CodeResources from package-manifest.
Attachment #8471616 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8472478 [details] [diff] [review]
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.
Attachment #8472370 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Created attachment 8473005 [details] [diff] [review]
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.
Attachment #8472478 - Attachment is obsolete: true
Attachment #8473005 - Flags: feedback?(benjamin)
(Assignee)

Comment 11

3 years ago
Created attachment 8473006 [details] [diff] [review]
Enable v2 signing
Attachment #8473006 - Flags: review?(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.
Attachment #8473006 - Flags: review?(bhearsum) → review+
(Assignee)

Updated

3 years ago
No longer depends on: 1046924
(Assignee)

Updated

3 years ago
Depends on: 1046924
(Assignee)

Updated

3 years ago
Depends on: 1053820
(Assignee)

Updated

3 years ago
Depends on: 1053822
(Assignee)

Updated

3 years ago
Depends on: 1053838
(Assignee)

Updated

3 years ago
No longer depends on: 1053838
(Assignee)

Updated

3 years ago
No longer depends on: 1053822
(Assignee)

Comment 13

3 years ago
Created attachment 8473682 [details] [diff] [review]
Patch

Renaming the .lproj directory to en-US.lproj would break a bunch of l10n scripts, so un-renaming to en.lproj.
Attachment #8473005 - Attachment is obsolete: true
Attachment #8473005 - Flags: feedback?(benjamin)
Attachment #8473682 - Flags: feedback?(benjamin)
(Assignee)

Updated

3 years ago
Depends on: 1055774
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.
Attachment #8473682 - Flags: feedback?(benjamin) → feedback-
(Assignee)

Comment 15

3 years ago
Created attachment 8479990 [details] [diff] [review]
Patch

Addressed feedback. Still waiting for test harnesses to be updated and start passing before requesting review.
Attachment #8473682 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8479990 - Attachment description: bug1047584 → Patch
(Assignee)

Updated

3 years ago
Depends on: 1059504

Comment 16

3 years ago
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?
(Assignee)

Comment 17

3 years ago
(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.
Depends on: 1064523
(Assignee)

Updated

3 years ago
Depends on: 1064910
(Assignee)

Comment 18

3 years ago
Created attachment 8487476 [details] [diff] [review]
Patch

Fixed OSX debug build failure.
Attachment #8479990 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
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
Attachment #8487476 - Flags: review?(benjamin)
(Assignee)

Updated

3 years ago
Depends on: 1066123
Attachment #8487476 - Flags: review?(benjamin) → review+
Depends on: 1068439
(Assignee)

Comment 20

3 years ago
https://hg.mozilla.org/projects/oak/rev/6faa732a5a82
https://hg.mozilla.org/projects/oak/rev/012e80a2ad45
Depends on: 1071465
Depends on: 1072722
Blocks: 1074002
Blocks: 1074019
Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/3ff20d4c7f39
https://hg.mozilla.org/integration/fx-team/rev/fbc1ceea4d6f
https://hg.mozilla.org/mozilla-central/rev/3ff20d4c7f39
https://hg.mozilla.org/mozilla-central/rev/fbc1ceea4d6f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Depends on: 1075169
Depends on: 1075483

Updated

3 years ago
Depends on: 1075981

Updated

3 years ago
Blocks: 1076370
(Assignee)

Updated

3 years ago
Depends on: 1075691
(Assignee)

Updated

3 years ago
Depends on: 1076977
(Assignee)

Updated

3 years ago
Depends on: 1076941
(Assignee)

Updated

3 years ago
Depends on: 1077099
Is OSX actually happy with Firefox loading the binary browsercomps component from the resources directory (which, even if OSX does it, feels very wrong)?
It is from a signing point of view. We might change its location to something more suitable after this initial work has been uplifted.
(Assignee)

Comment 25

3 years ago
Created attachment 8502897 [details] [diff] [review]
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
Attachment #8502897 - Flags: review+
Attachment #8502897 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8502897 - Flags: approval-mozilla-aurora?
Depends on: 1080395
(Assignee)

Comment 26

3 years ago
Created attachment 8503134 [details] [diff] [review]
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
Attachment #8502897 - Attachment is obsolete: true
Attachment #8503134 - Flags: review+
(Assignee)

Comment 27

3 years ago
Created attachment 8503238 [details] [diff] [review]
Patch for aurora

Now also with patch from bug 1080395.
Attachment #8503134 - Attachment is obsolete: true
Attachment #8503238 - Flags: review+
(Assignee)

Comment 28

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

Please see comment 25 for approval request comment.
Attachment #8503238 - Flags: approval-mozilla-aurora?
Comment on attachment 8503238 [details] [diff] [review]
Patch for aurora

I believe that the patch from bug 1075169 isn't included. Still checking others
Attachment #8503238 - Flags: approval-mozilla-aurora?
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));
     }
(Assignee)

Comment 31

3 years ago
Created attachment 8503413 [details] [diff] [review]
Patch for aurora

This should fix it.
Attachment #8503238 - Attachment is obsolete: true
Attachment #8503413 - Flags: review+
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
Attachment #8503413 - Flags: approval-mozilla-aurora?
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.
Attachment #8503413 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(lhenry)
Flags: needinfo?(jbecerra)
[Tracking Requested - why for this release]:
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → fixed
status-firefox-esr31: --- → affected
tracking-firefox34: --- → +
Comment on attachment 8503413 [details] [diff] [review]
Patch for aurora

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/42dba12fb601
status-firefox34: affected → fixed

Updated

3 years ago
Blocks: 1074025

Updated

3 years ago
Blocks: 1084578

Comment 36

2 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 37

2 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 38

2 years ago
Filed Bug 1103566.
(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?
(Assignee)

Comment 40

2 years ago
(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.
Release noted as "Firefox signed by Apple OS X version 2 signature".
relnote-firefox: --- → 34+
(Assignee)

Updated

2 years ago
Blocks: 1101331
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?
Flags: needinfo?(lhenry)
Flags: needinfo?(jbecerra)
You need to log in before you can comment on or make changes to this bug.