Cleanup package-manifest after OSX v2 signing changes

RESOLVED FIXED in Firefox 36

Status

()

Firefox
Installer
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: spohl, Assigned: spohl)

Tracking

Trunk
Firefox 36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
After all the patches to get v2 signing working on OSX, package-manifest deserves a bit of a cleanup.
(Assignee)

Comment 1

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

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+
(Assignee)

Comment 3

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

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)
(Assignee)

Comment 5

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

Updated

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

Comment 7

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

Comment 8

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

Comment 9

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

Comment 10

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

Updated

3 years ago
Depends on: 1100424
(Assignee)

Comment 13

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

Comment 14

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

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)
(Assignee)

Comment 16

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

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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Updated

3 years ago
Blocks: 1102013
(Assignee)

Updated

3 years ago
Blocks: 1102033
(Assignee)

Updated

3 years ago
Blocks: 1102037
(Assignee)

Comment 20

3 years ago
Created attachment 8526018 [details] [diff] [review]
Followup for uninstall helper.exe

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+
(Assignee)

Comment 21

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e611d05200ac
https://hg.mozilla.org/mozilla-central/rev/e611d05200ac
Flags: needinfo?(rlb)
Depends on: 1223979
You need to log in before you can comment on or make changes to this bug.