Closed Bug 1096494 Opened 10 years ago Closed 10 years ago

Cleanup package-manifest after OSX v2 signing changes

Categories

(Firefox :: Installer, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: spohl, Assigned: spohl)

References

Details

Attachments

(2 files, 3 obsolete files)

After all the patches to get v2 signing working on OSX, package-manifest deserves a bit of a cleanup.
Attached patch Patch (obsolete) — Splinter Review
Try is green with this patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4a1a75b65041
Attachment #8520084 - Flags: review?(robert.strong.bugs)
Comment on attachment 8520084 [details] [diff] [review]
Patch

Please add a comment to the top of package-manifest.in that explains @RESPATH@ and @BINPATH@ similar to the one at
http://mxr.mozilla.org/mozilla-central/source/b2g/installer/removed-files.in#1

It would be a good thing to diff before and after bundles just to be safe.

r=me with those two things being done
Attachment #8520084 - Flags: review?(robert.strong.bugs) → review+
Attached patch Patch (obsolete) — Splinter Review
Sorry, I could have sworn that I diffed before and after bundles, but I must have grabbed two identical bundles. Turns out that the previous patch accidentally re-enabled the creation of .chk files under Contents/MacOS and also placed the removed-files and precomplete files back in Contents/MacOS. This patch addresses this in packager.py and also adds the comment requested in the previous review.
Attachment #8520084 - Attachment is obsolete: true
Attachment #8521552 - Flags: review?(robert.strong.bugs)
Comment on attachment 8521552 [details] [diff] [review]
Patch

>diff --git a/toolkit/mozapps/installer/packager.py b/toolkit/mozapps/installer/packager.py
>--- a/toolkit/mozapps/installer/packager.py
>+++ b/toolkit/mozapps/installer/packager.py
>@@ -344,28 +345,29 @@ def main():
>...
> 
>     # shlibsign libraries
>     if launcher.can_launch():
>-        for lib in SIGN_LIBS:
>-            libbase = mozpack.path.join(binpath, '%s%s') \
>-                % (buildconfig.substs['DLL_PREFIX'], lib)
>-            libname = '%s%s' % (libbase, buildconfig.substs['DLL_SUFFIX'])
>-            if copier.contains(libname):
>-                copier.add(libbase + '.chk',
>-                           LibSignFile(os.path.join(args.destination,
>-                                                    libname)))
>+        if not mozinfo.isMac:
>+            for lib in SIGN_LIBS:
>+                libbase = mozpack.path.join(respath, '%s%s') \
>+                    % (buildconfig.substs['DLL_PREFIX'], lib)
>+                libname = '%s%s' % (libbase, buildconfig.substs['DLL_SUFFIX'])
>+                if copier.contains(libname):
>+                    copier.add(libbase + '.chk',
>+                               LibSignFile(os.path.join(args.destination,
>+                                                        libname)))
How are the .chk files created for Mac with this exclusion?

Clearing r? for an answer to the above
Attachment #8521552 - Flags: review?(robert.strong.bugs)
As of the landing of all the v2 patches, these files are no longer created. The reason is that they could no longer be under Contents/MacOS and moving them to Contents/Resources would have broken the signature verification anyway.

The creation of these files was accidentally re-enabled by the previous patch because BINPATH was changed to point to Contents/MacOS again. This allowed packager.py to find the dylibs again, hence it created the .chk files again and placed them next to the dylibs in Contents/MacOS.
Attachment #8521552 - Flags: review?(robert.strong.bugs)
Comment on attachment 8521552 [details] [diff] [review]
Patch

(In reply to Stephen Pohl [:spohl] from comment #5)
> As of the landing of all the v2 patches, these files are no longer created.
> The reason is that they could no longer be under Contents/MacOS and moving
> them to Contents/Resources would have broken the signature verification
> anyway.
> 
> The creation of these files was accidentally re-enabled by the previous
> patch because BINPATH was changed to point to Contents/MacOS again. This
> allowed packager.py to find the dylibs again, hence it created the .chk
> files again and placed them next to the dylibs in Contents/MacOS.
Did someone comment in a bug that these files are no longer needed?
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #6)
> Comment on attachment 8521552 [details] [diff] [review]
> Patch
> 
> (In reply to Stephen Pohl [:spohl] from comment #5)
> > As of the landing of all the v2 patches, these files are no longer created.
> > The reason is that they could no longer be under Contents/MacOS and moving
> > them to Contents/Resources would have broken the signature verification
> > anyway.
> > 
> > The creation of these files was accidentally re-enabled by the previous
> > patch because BINPATH was changed to point to Contents/MacOS again. This
> > allowed packager.py to find the dylibs again, hence it created the .chk
> > files again and placed them next to the dylibs in Contents/MacOS.
> Did someone comment in a bug that these files are no longer needed?

We've talked about this on IRC, but I think the only thing that made it into a bug comment was [1] and [2]. I had the impression that we basically didn't care about FIPS mode at the moment, but reading the bug comments again, this may just have been true to get v2 signing working and that we wanted FIPS mode to work again in the future.

Ted, can you clarify your comment [2]? If we need FIPS support in FF34, we'll have to scramble to get this in. Right now, we don't bundle any .chk files...

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1047584#c5
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1047584#c6
Flags: needinfo?(ted)
I just heard that the last beta build (before rc) is today, so if we do need FIPS mode in 34, we'll have to get into a mad scramble. Benjamin, would you know if we need FIPS support on OSX in FF34?
Flags: needinfo?(benjamin)
FIPS mode itself doesn't matter; it's a legal fiction and we don't bother with FIPS certification for Firefox . However, I'm not convinced whether NSS will work properly at all without those .chk files, and it may completely refuse to load HTTPS sites.
Flags: needinfo?(benjamin)
Richard, do you have any concerns here? To quickly summarize: To support v2 signatures on OSX, we had to move all the .chk files out of Contents/MacOS. Once we heard that we didn't necessarily support FIPS mode, the patches simply removed all .chk files from our .app bundles. This code is in our Firefox 34 beta builds and later (aurora, nightly) and we haven't had any reports that the missing .chk files would cause any problems. Do you have any concerns that NSS might not work properly without the .chk files (see comment 9)?
Flags: needinfo?(rlb)
I am quite confident that NSS will work fine without the .chk files, it will simply refuse to enter FIPS mode.
Flags: needinfo?(ted)
Comment on attachment 8521552 [details] [diff] [review]
Patch

We've had cases in the past where the .chk files were invalid and everything worked except FIPS mode and this has been the case on m-c, m-a, and m-b for quite awhile now so I tend towards agreeing with Ted that it works without those files. Please file a followup to make NSS look under resources on Mac for the .chk files. Thanks!
Attachment #8521552 - Flags: review?(robert.strong.bugs) → review+
Depends on: 1100424
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #12)
> Please file a followup to make NSS look under resources on Mac
> for the .chk files. Thanks!

Filed bug 1100424.
Attached patch Patch (obsolete) — Splinter Review
Updated for current trunk, carrying over r+. Landed on mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/132909245ca8
Attachment #8521552 - Attachment is obsolete: true
Attachment #8523983 - Flags: review+
sorry had to back this out since this push is causing a merge conflict when merging mozilla-inbound to mozilla-central

merging browser/installer/package-manifest.in                                                                                                                                                                                                 
3 files to edit
merging browser/installer/package-manifest.in failed!  

could you take a look at this, seems this conflicts with bug 1044736
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch PatchSplinter Review
Trying this again. Updated for current trunk, carrying over r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff71fa57c6c9
Attachment #8523983 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8524703 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ff71fa57c6c9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Blocks: 1102013
Blocks: 1102033
Blocks: 1102037
Minor followup. Noticed that this should point to @BINPATH@ even though it doesn't currently break anything since @RESPATH@ and @BINPATH@ are equivalent on Windows.
Attachment #8526018 - Flags: review?(robert.strong.bugs)
Attachment #8526018 - Flags: review?(robert.strong.bugs) → review+
Flags: needinfo?(rlb)
Depends on: 1223979
You need to log in before you can comment on or make changes to this bug.