Over in bug 400296 we're working on signing our OS X builds. One of the requirements of doing that is a CodeResources file that describes all of the files that are optionally present or can change but must be signed, and all of the files that explicitly do not need to be signed. We went through an iteration of this last year, and I've attached what we came up with then. This file needs to be present in MOZ_PKG_DIR prior to MOZ_SIGN_PREPARED_PACKAGE_CMD beuing called. Additionally, we need to set SIGN_INCLUDES/SIGN_EXCLUDES for Mac, and set MOZ_INTERNAL_SIGNING_FORMAT to dmg. Once the signing command gets the pkg dir and the include/exclude list, it will package up the files, probably in a tarball, and send them to the signing server to be signed. Because the Mac codesign tool requires a complete .app when it signs, we can't send these files one by one like we do on Windows. We _could_ potentially do this as part of the external signing instead, and send a dmg file to the signing server, but then we have to unpack/pack DMGs on the server, which duplicates a bunch of build system stuff, and will cause problems the next time we change any of that logic on trunk. Erick, Ted suggested that the CodeResources file itself should live here: http://mxr.mozilla.org/mozilla-central/source/browser/installer/ You should set MOZ_INTERNAL_SIGNING_FORMAT, SIGN_INCLUDES, and SIGN_EXCLUDES in this file, inside the ifeq() block for Darwin: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/signing.mk#44 I _think_ those are the only changes that will be necessary. To test you'll need to do a build and then run 'make package' with MOZ_SIGN_CMD set. If you need a hand with any of that, let me know!

Looking into this a bit more I see that we actually need to list _all_ files, not just optional/omitted ones. Ted says that because of this we should generate the CodeResources file from browser/installer/package-manifest.in. We should probably write a Python script to do that after package-manifest gets processed. I'm not sure where/when we can call it from the build system though. Attached is a sample CodeResources file that I found on a system Syed was using to work on mac signing last year. It probably needs a bit of updating because of things like omni.jar changing to omni.ja, and other file changes.

The package manifest for Firefox is generated here, FWIW: http://mxr.mozilla.org/mozilla-central/source/browser/installer/Makefile.in#104

11:45 < bhearsum> ted: so, i was just looking into how to generate the mac CodeResources from package-manifest and i noticed that package-manifest has no mention of omni.jar, and seems to list the files from it out instead 11:45 < bhearsum> is that what's supposed to happen? ed> bhearsum: um 12:02 <@ted> bhearsum: i think it's because we use package-manifest to stage everything into a directory, and then omnijar the bits from there 12:03 <@ted> mwu would probably be able to tell you better 12:03 < bhearsum> ah 12:03 <@khuey> omni.ja is magical 12:03 <@khuey> it gets created by packager.mk 12:03 <@ted> bhearsum: what we could do is add some more #ifdefs around that 12:03 <@ted> #ifdef ALREADY_STAGED 12:03 <@ted> omni.ja 12:03 <@ted> #else 12:03 <@ted> ... existing omnijar contents 12:03 < bhearsum> ah, clever 12:04 <@ted> i'd really like to try to avoid duplicating this file's contents 12:04 < bhearsum> yeah, me too 12:04 <@ted> it's hard enough to keep it up-to-date 2:06 < bhearsum> ok, and then we need some Makefile goo to interpret it a second time, i guess, or maybe the script that generates CodeResources will do that 12:06 <@ted> yeah, either way 12:06 < bhearsum> alright 12:06 <@ted> you can import Preprocessor.py as a python module, probably 12:06 <@ted> and just do it internally 12:06 < bhearsum> oh cool 12:07 < bhearsum> that sounds ideal

We have noticed that if a pathname is specified without a '$' to anchor the end of the path, codesign will recursively sign all of the files below that path. Using this, I think we can use a static CodeResources file and just make modifications to omissions if we have files we need to omit. This will make the file much more human-readable, and human-maintainable. It can stay the same build-to-build and we shouldn't need to generate it automatically. I'll attach an example of what I think the resulting CodeResources file will look like. There are a few files that 'find' revealed that aren't included in the sample CodeResources file that bhearsum gave me last month. I'll include a list for inspection. We need to decide if we need to explicitly omit these files.

Allowing codesign to recursively sign files below a specified directory significantly shrinks this file. It should be static and human-maintainable.

The files which are in the package but aren't currently listed in the CodeResources file are: MacOS/libmozutils.dylib *.chk (what are these files? they seem to have the same names as some of the .dylib files) MacOS/libplugin_child_interpose.dylib ./MacOS/jsloader/resource (this directory is empty, will it ever have something in it? if so, does it need to be excluded from signing?) If any of these files should *not* be signed, we need to explicitly omit them in the CodeResources file.

(In reply to Erick Dransch [:edransch] from comment #6) > *.chk (what are these files? they seem to have the same names as some of the > .dylib files) These are the NSS chk files. Since "signing" in this context means "generate a checksum and put it in CodeResources", these should be signed IMO.

(In reply to Erick Dransch [:edransch] from comment #6) > The files which are in the package but aren't currently listed in the > CodeResources file are: > > MacOS/libmozutils.dylib > MacOS/libplugin_child_interpose.dylib I can't imagine why we wouldn't want to sign these. > ./MacOS/jsloader/resource (this directory is empty, will it ever have > something in it? if so, does it need to be excluded from signing?) I'm not sure what this is, Ted?

I have no idea what that is.

Well, it's empty for now so there's no reason to exclude it yet. So, it sounds like the attached CodeResources is what we need.

Patch to build system to add put the condensed CodeResources file into the package.

Comment on attachment 598320 [details] [diff] [review] patch to buildsystem Review of attachment 598320 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK to me, but I'm not an expert on the style guidelines here.

This is the signed CodeResources file that results from signing the latest Nightly (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-02-17-03-12-27-mozilla-central/firefox-13.0a1.en-US.mac.dmg) with the attached Resource Rules. We should verify that everything that should be signed is, and that there is nothing that signed that shouldn't be.

Comment on attachment 598320 [details] [diff] [review] patch to buildsystem Review of attachment 598320 [details] [diff] [review]: ----------------------------------------------------------------- I don't know anything about the CodeResources file itself, so I'm assuming bhearsum or someone else is going to review that. ::: browser/app/Makefile.in @@ +202,5 @@ > > libs repackage:: $(PROGRAM) > $(MKDIR) -p $(DIST)/$(MOZ_MACBUNDLE_NAME)/Contents/MacOS > rsync -a --exclude "*.in" $(srcdir)/macbuild/Contents $(DIST)/$(MOZ_MACBUNDLE_NAME) --exclude English.lproj > + cd $(DIST)/$(MOZ_MACBUNDLE_NAME) && ln -sf ./_CodeSignature/CodeResources ./Contents/CodeResources ; cd - You don't need a final cd here, each line of a make recipe runs in a separate shell.

If you need further review, you'll want to ask khuey or glandium, since I'm likely to not be available.

Looks like we need to adjust MOZ_INTERNAL_SIGNING_FORMAT for Mac, too. Erick, I think this will just be the change in http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/signing.mk#51 that we talked about earlier. After rereading the packager.mk code I realized that MOZ_SIGN_CMD will get passed $(MOZ_PKG_DIR), which is the directory containing the .app file.

Previous patch was the wrong file.

Comment on attachment 599285 [details] [diff] [review] Add MOZ_INTERNAL_SIGNING_FORMAT for Darwin builds Review of attachment 599285 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. We'll have to get a build system peer to review it after testing is done.

<braindump> It turns out that in the tests I ran, the buildsystem is passing 'm-cen-osx64-ntly/build/obj-firefox/i386/dist/firefox' to the server. We need it to pass 'm-cen-osx64-ntly/build/obj-firefox/i386/dist/FirefoxNightly.app'. I haven't tried digging into how to accomplish this yet. </braindump>

Hmm, I think that's fallout from https://bugzilla.mozilla.org/show_bug.cgi?id=696436, which started using MOZ_MACBUNDLE_NAME when staging mac packages instead of APP_NAME. Over here, MOZ_PKG_DIR is passed to the internal signing command: http://hg.mozilla.org/mozilla-central/file/tip/toolkit/mozapps/installer/packager.mk#l560. And over here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#83 MOZ_PKG_DIR is set to MOZ_APP_NAME. It seems to me that MOZ_PKG_DIR might need to be set to MOZ_MACBUNDLE_NAME for Darwin platforms....or maybe we should be ifdef'ing here instead: http://hg.mozilla.org/mozilla-central/file/tip/toolkit/mozapps/installer/packager.mk#l559 Kyle, any advice on how to get the correct directory passed to MOZ_SIGN_PREPARED_PACKAGE_CMD here: http://hg.mozilla.org/mozilla-central/file/tip/toolkit/mozapps/installer/packager.mk#l559 ?

Modified MAKE_PACKAGE to use MOZ_MACBUNDLE_NAME for signing on mac builds. Still use MOZ_PKG_DIR for other platforms.

Comment on attachment 600477 [details] [diff] [review] m-c patch to integrate mac signing Review of attachment 600477 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK to me, modulo a successful try run.

Autoland Patchset: Patches: 600477 Branch: mozilla-central => try Error applying patch 600477 to mozilla-central. patching file browser/app/Makefile.in Hunk #1 FAILED at 197 1 out of 1 hunks FAILED -- saving rejects to file browser/app/Makefile.in.rej abort: patch failed to apply Could not apply and push patchset:

Updated diff to avoid bitrot

Autoland Patchset: Patches: 600514 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=cbb75a3e52f2 Try run started, revision cbb75a3e52f2. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=cbb75a3e52f2

Try run for cbb75a3e52f2 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=cbb75a3e52f2 Results (out of 49 total builds): success: 42 warnings: 3 failure: 4 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-cbb75a3e52f2

Comment on attachment 600514 [details] [diff] [review] m-c patch to integrate mac signing Review of attachment 600514 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, considering that it passed on try, and the Windows build still got signed. Needs final review from Kyle though.

Comment on attachment 600514 [details] [diff] [review] m-c patch to integrate mac signing 302 ted

Comment on attachment 600514 [details] [diff] [review] m-c patch to integrate mac signing We retested this patch with the new mac signing servers yesterday and it doesn't actually seem to be working. I think we're signing the wrong .app folder, or the right one at the wrong time.

During our testing we ran into some odd results: 1) The '.app' directory that is signed is not the '.app' that is included in the '.dmg'. The temp directory that is being used to generate the '.dmg' gets populated via an rsync from 'universal/firefox'. However, the folder we signed is in 'm-cen-osx64/build/obj-firefox/i386/dist' With a search through mxr, it looks like the rsync is happening here: http://mxr.mozilla.org/mozilla-central/source/build/package/mac_osx/pkg-dmg#569 Though I might be mistaken. We are not sure how to ensure that the directory that is signed is the same one that's packaged in to the dmg. Should we try to sign the package after it's been generated (via mounting the dmg, signing, and repackaging)? Or can we sign the correct directory before the '.dmg' is generated?

Currently we're stuck at the following problem: The CodeResources file is not being copied into the universal/firefox/ '.app' folder. The single arch '.app' directory *does* contain the CodeResources file. It is added in https://bugzilla.mozilla.org/attachment.cgi?id=600514&action=diff#a/browser/app/Makefile.in_sec1 I'm not sure where in the build process I should copy the CodeResources into the universal/firefox/ '.app' folder. Ted: Do you have any suggestions for how we can ensure that the CodeResources file makes it into the '.app' folder?

One of our ideas was to copy the file in packager.mk, but we had no luck with that. http://hg.mozilla.org/users/edransch_mozilla.com/mozilla-central/rev/7f4b84d34c8b#l1.12

Presumably you just need to list it in the package manifest, probably somewhere around here: http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#18

Updated patch for the build-system. Our tests now generate signed dmg's that validate using 'codesign -vv'.

Comment on attachment 608840 [details] [diff] [review] Patch to build-system Review of attachment 608840 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/Makefile.in @@ +198,5 @@ > > libs repackage:: $(PROGRAM) > $(MKDIR) -p $(DIST)/$(MOZ_MACBUNDLE_NAME)/Contents/MacOS > rsync -a --exclude "*.in" $(srcdir)/macbuild/Contents $(DIST)/$(MOZ_MACBUNDLE_NAME) --exclude English.lproj > + cd $(DIST)/$(MOZ_MACBUNDLE_NAME) && ln -sf ./_CodeSignature/CodeResources ./Contents/CodeResources I'm not sure whether this is necessary or appropriate here given what we've learned about the build system lately, Ted will surely know though =).

Comment on attachment 608840 [details] [diff] [review] Patch to build-system Review of attachment 608840 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/Makefile.in @@ +198,5 @@ > > libs repackage:: $(PROGRAM) > $(MKDIR) -p $(DIST)/$(MOZ_MACBUNDLE_NAME)/Contents/MacOS > rsync -a --exclude "*.in" $(srcdir)/macbuild/Contents $(DIST)/$(MOZ_MACBUNDLE_NAME) --exclude English.lproj > + cd $(DIST)/$(MOZ_MACBUNDLE_NAME) && ln -sf ./_CodeSignature/CodeResources ./Contents/CodeResources You could probably just commit this symlink in browser/app/macbuild, but you'd have to fiddle with the rsync above to make that sync the right bits. (Hg handles symlinks on anything but Windows fine.) ::: browser/app/macbuild/Contents/_CodeSignature/CodeResources @@ +27,5 @@ > + <key>^MacOS/active-update.xml$</key><dict> > + <key>omit</key> > + <true/> > + </dict> > + <key>^MacOS/defaults/.*</key><dict> Don't we ship at least one file in defaults with the app? If so, we should probably sign that. ::: toolkit/mozapps/installer/packager.mk @@ +561,5 @@ > > ifdef MOZ_SIGN_PREPARED_PACKAGE_CMD > +ifeq (Darwin, $(OS_ARCH)) > +MAKE_PACKAGE = $(PREPARE_PACKAGE) && cd ./$(PKG_DMG_SOURCE) && $(MOZ_SIGN_PREPARED_PACKAGE_CMD) \ > + $(MOZ_MACBUNDLE_NAME) && cd $(_ABS_DIST) && $(INNER_MAKE_PACKAGE) The line-wrapping here is a little goofy. You should probably put $(MOZ_SIGN_PREPARED_PACKAGE_CMD) $(MOZ_MACBUNDLE_NAME) all one one line.

(In reply to Ted Mielczarek [:ted] from comment #37) > ::: browser/app/macbuild/Contents/_CodeSignature/CodeResources > @@ +27,5 @@ > > + <key>^MacOS/active-update.xml$</key><dict> > > + <key>omit</key> > > + <true/> > > + </dict> > > + <key>^MacOS/defaults/.*</key><dict> > > Don't we ship at least one file in defaults with the app? If so, we should > probably sign that. The only file in there is channel-prefs.js, and I don't think we want to sign it because modifying that file is how QA does their pre-release update testing. It's unfortunate, because in an ideal world we would want to sign this.

Address Ted's review.

Because of a potential 10.8 release in early June, we're targeting Firefox 13 as our first release with signed Mac builds. This means that this bug will need to jump the trains pretty quickly. Once we solidify the signing server situation, we need to land this immediately on Nightly. If there are no immediate problems, I think we should push to Aurora a day or two later, and Beta very soon after. I'm hoping to get this on Nightly by midweek. The patch cleanly applies to all three branches, so it's just a matter of landing it when we're ready.

I retested this patch last night in my user repo and it worked fine. I'm hoping to land it today or tomorrow.

Everything seems to be good on try: https://tbpl.mozilla.org/?tree=Try&rev=2e59bcb08384 The only non-hidden orange is a known intermittent one. At this point, I'm just waiting for us to figure out our Developer ID situation before pushing this to mozilla-central.

Comment on attachment 611452 [details] [diff] [review] Patch to build-system This has been tested on try, and the metadiff is very small. Can someone review this ASAP so we can get mac build signing enabled?

Comment on attachment 611452 [details] [diff] [review] Patch to build-system Review of attachment 611452 [details] [diff] [review]: ----------------------------------------------------------------- Have fun kids. Don't stay out plus midnight, and shut the garage door when you come home.

Comment on attachment 611452 [details] [diff] [review] Patch to build-system http://hg.mozilla.org/mozilla-central/rev/b038090f07c2 I forced a new nightly so we can have testing ASAP.

Looks like we never tested this for l10n. Getting the following in a nightly repack: Deoptimized 0/16 in ./omni.ja 2012-05-22 18:47:39,068 - f295d645c4d1603a69258251ed4c66827823d0b4: processing FirefoxNightly.app.tar.gz on https://mac-signing2.srv.releng.scl3.mozilla.com:9100 2012-05-22 18:47:39,788 - f295d645c4d1603a69258251ed4c66827823d0b4: uploading for signing 2012-05-22 18:48:18,010 - f295d645c4d1603a69258251ed4c66827823d0b4: OK 2012-05-22 18:48:18,010 - Copying FirefoxNightly.app.tar.gz to cache /builds/slave/m-cen-osx64-l10n-ntly/signing_cache/dmg/f295d645c4d1603a69258251ed4c66827823d0b4 mktemp -d pkg-dmg.10479.XXXXXXXX pkg-dmg.10479.rGNcRanp mkdir pkg-dmg.10479.rGNcRanp/stage rsync -a --copy-unsafe-links firefox/ pkg-dmg.10479.rGNcRanp/stage rsync: link_stat "/builds/slave/m-cen-osx64-l10n-ntly/build/mozilla-central/dist/firefox/." failed: No such file or directory (2) rsync error: some files could not be transferred (code 23) at /SourceCache/rsync/rsync-42/rsync/main.c(992) [sender=2.6.9] /builds/slave/m-cen-osx64-l10n-ntly/build/mozilla-central/build/package/mac_osx/pkg-dmg: rsync failed (cleaning up) make[1]: *** [repackage-zip] Error 1 make: *** [repackage-zip-sl] Error 2

Is this an error on the signing server?

> Is this an error on the signing server? Now I see that it probably isn't. This code isn't in the tree, though. So I suppose it must be in some separate part of the build system.

I think the problem here is that we cd to $(_ABS_DIST) after running $(MOZ_SIGN_PREPARED_PACKAGE_CMD). For l10n, I _think_ the previous directory was $(_ABS_DIST)/l10n-stage. I don't think we have any existing variable that we can sub in here that works for en-US *and* l10n, so I think we need to store the cwd and cd back to it...

I pushed this patch to try as a potential fix: http://hg.mozilla.org/try/rev/69894b8c339e It failed with: Traceback (most recent call last): File "/builds/slave/try-osx64-dbg/tools/release/signing/signtool.py", line 189, in <module> main() File "/builds/slave/try-osx64-dbg/tools/release/signing/signtool.py", line 171, in main if remote_signfile(options, url, f, fmt, token, dest): File "/builds/slave/try-osx64-dbg/tools/release/signing/signing.py", line 485, in remote_signfile filehash = sha1sum(filename) File "/builds/slave/try-osx64-dbg/tools/release/signing/signing.py", line 162, in sha1sum fp = open(f, 'rb') IOError: [Errno 2] No such file or directory: 'firefox-15.0a1.en-US.mac64.dmg' make[3]: *** [make-package-internal] Error 1 make[2]: *** [make-package] Error 2 make[1]: *** [default] Error 2 make: *** [package] Error 2 ...which I don't understand because the only new code that's being executed (as far as I can tell) is 'old_cwd=`pwd`'. Any guidance or advice here would be appreciated.

Comment on attachment 611452 [details] [diff] [review] Patch to build-system I'm out of ideas for fixing this tonight so I had to back this out: http://hg.mozilla.org/mozilla-central/rev/b038090f07c2

(In reply to Ben Hearsum [:bhearsum] from comment #51) > Comment on attachment 611452 [details] [diff] [review] > Patch to build-system > > I'm out of ideas for fixing this tonight so I had to back this out: > http://hg.mozilla.org/mozilla-central/rev/b038090f07c2 That's the original landing. I don't see a backout in mozilla-central.

(In reply to Steven Michaud from comment #48) > > Is this an error on the signing server? > > Now I see that it probably isn't. > > This code isn't in the tree, though. So I suppose it must be in some > separate part of the build system. It's in https://github.com/mozilla/mozilla-central/blob/master/build/package/mac_osx/pkg-dmg

(In reply to Mark Banner (:standard8) from comment #52) > (In reply to Ben Hearsum [:bhearsum] from comment #51) > > Comment on attachment 611452 [details] [diff] [review] > > Patch to build-system > > > > I'm out of ideas for fixing this tonight so I had to back this out: > > http://hg.mozilla.org/mozilla-central/rev/b038090f07c2 > > That's the original landing. I don't see a backout in mozilla-central. d'oh! Turns out my push failed last night, and I didn't double check before heading out. There's not much point in backing out now, unless there's no solution by the end of the day, so I'm going to try to push the backout again just yet.

I just pushed another potential fix to try: I did a quick test in an existing l10n objdir and it _seemed_ to work. It's very difficult to do a full test by hand, but once the try build is finished I may be able to coerce a buildbot into doing what I want.

Oops, forgot to include a link to the changeset: http://hg.mozilla.org/try/rev/3584e25902a7

http://hg.mozilla.org/try/rev/3584e25902a7 seems to work, but it's an awful hack. I'm testing a different patch that copies the $(PACKAGE_BASE_DIR) technique the l10n already uses for update-packaging (https://github.com/mozilla/mozilla-central/blob/master/tools/update-packaging/Makefile.in#L16, https://github.com/mozilla/mozilla-central/blob/master/toolkit/locales/l10n.mk#L130): http://hg.mozilla.org/try/rev/c414283ef2db.

(In reply to comment #53) Actually, the code that produced the output in comment #46 is here: https://github.com/catlee/tools/blob/master/release/signing/signing.py I found this by searching in Google on '"uploading for signing" site:github.com' :-)

(In reply to Steven Michaud from comment #58) > (In reply to comment #53) > > Actually, the code that produced the output in comment #46 is here: > https://github.com/catlee/tools/blob/master/release/signing/signing.py > > I found this by searching in Google on '"uploading for signing" > site:github.com' :-) Yeah, but the problem is with the args passed to signtool.py, not with signtool.py itself.

A try run of this is still running here: https://tbpl.mozilla.org/?tree=Try&rev=e380616d7d5d But the Mac build already finished packaging/uploading, and I was able to do a repack with it without issue. The completed Linux opt build ran 'l10n-check' for both pretty and non-pretty builds, and that passed -- which is a pretty good indicator that linux repacks are fine. The Windows opt build also run the l10n check steps successfully. I'm pretty confident this will get us in a working state without busting anything.

Comment on attachment 626501 [details] [diff] [review] add PACKAGE_BASE_DIR to packager.mk, override in l10n.mk, to fix mac repacks http://hg.mozilla.org/mozilla-central/rev/320b16daa7c0. I triggered new nightlies, too.

This seems to have fixed l10n.

Comment on attachment 611452 [details] [diff] [review] Patch to build-system [Approval Request Comment] User impact if declined: Builds won't run on OS X 10.8 Testing completed (on m-c, etc.): Try run, manual testing by me, QA, and smichaud. Risk to taking this patch (and alternatives if risky): Potential for uncaught & unpredicted issues. Worst case scenario: builds don't work on 10.8 (as far as I know, there's no risk of this for earlier OS X versions). String or UUID changes made by this patch: None

On nightlytest, Marcia's running into issues with invalid code signatures after update, slower update times, and browser restarts not occurring after accepting an update (depending on OS version and whether we're updating from signed to signed or unsigned to unsigned). I'm wondering if we're running into new issues caused by bug 307181 landing on the same day. Tomorrow it'll be pref'd off. We agreed it would make sense to put off enabling this for tomorrow's builds until we have a full test matrix filled out and can figure out if there are any critical issues. Another test run without background updates enabled may also make sense.

> invalid code signatures after update That's actually a big problem. Sounds like we'll need to be able to resign FF after an update. I don't know whether we can do that now, or how we should go about it (talk directly to the signing server from each machine that's doing an update?).

(In reply to Steven Michaud from comment #65) > > invalid code signatures after update > > That's actually a big problem. > > Sounds like we'll need to be able to resign FF after an update. I don't > know whether we can do that now, or how we should go about it (talk directly > to the signing server from each machine that's doing an update?). Based on https://bugzilla.mozilla.org/show_bug.cgi?id=758046#c0, this is an easy fix. We just need to exclude removed-files from the signature. No need panic about this one, it's a few liner fix.

But what about files the update might change? If they've been signed the signature would become invalid. We may need to *not* sign the parts of Firefox that might change in an update.

The MAR updates all files that have changed -- including CodeResources. This means that the signature is still valid, modulo-removed-files. removed-files is special, in that it isn't included in the MAR (along with a few other files: channel-prefs.js, update.manifest and updatev2.manifest).

Oh, OK. Thanks! Big sigh of relief!! :-) I suppose I should go find my Hitchhiker's Guide.

Comment on attachment 626644 [details] [diff] [review] add omission for removed-files http://hg.mozilla.org/mozilla-central/rev/0024af60b664 I triggered a new mac nightly build, too. Once that's done one can download the previous nightly (which is still running, after a respin for something else), and test the partial update with it. I'll provide an excat link to the build that can be tested with when I can.

OK, builds are out. You can use this build to test with for now: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-05-23-16-43-48-mozilla-central/firefox-15.0a1.en-US.mac.dmg I just did a test of my own, and unfortunately, codesign threw errors about *other* files this time. Specifically, updates.xml and channel-prefs.js. 1) Download http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-05-23-16-43-48-mozilla-central/firefox-15.0a1.en-US.mac.dmg with Safari 2) Mount the DMG, drag FirefoxNightly to the Desktop 3) Open a Terminal, run the following: cd ~/Desktop codesign -vv FirefoxNightly.app ...which returned: FirefoxNightly.app/: valid on disk FirefoxNightly.app/: satisfies its Designated Requirement and then: cat FirefoxNightly.app/Contents/MacOS/application.ini ...which returned: BuildID=20120523164348 4) Edit FirefoxNightly.app/Contents/MacOS/defaults/pref/channel-prefs.js to change to the "nightlytest" channel. 5) Run FirefoxNightly by double clicking it, Go to Nightly -> About to check for updates 6) Got a 120K update (size expected because of such little code change between this and the previous Nightly), indicating a partial MAR. 7) Clicked "Restart to Update", saw updater progress bar go by. 8) Back in the Terminal, ran: codesign -vv FirefoxNightly.app ...and got: FirefoxNightly.app/: a sealed resource is missing or invalid In architecture: i386 resource added: /Users/bhearsum/Desktop/FirefoxNightly.app/Contents/MacOS/updates.xml resource modified: /Users/bhearsum/Desktop/FirefoxNightly.app/Contents/MacOS/defaults/preferences/channel-prefs.js. These are both files that I *thought* were excluded already, but apparently we have a bit more work to do on the CodeResources file. This has gone far enough for now though, and I'm currently working on backing out all three patches, and retriggering yet another nightly. In the morning, after the regularly scheduled nightly happens I will re-enable updates, which will be pointing at unsigned builds again. Thanks again to ted, gavin, and nthomas for helping work through this so far.

Comment on attachment 611452 [details] [diff] [review] Patch to build-system I'm going to combine these 3 patches into a single roll-up.

OK, I backed out all three in: http://hg.mozilla.org/mozilla-central/rev/f43e8d300f21. I triggered a new Mac nightly. We'll get this damn thing sorted out tomorrow, really!

I dug into this more this morning. I noticed a few things upon inspecting the build: * Contents/CodeResources is a copy of the unsigned CodeResources file, rather than a symlink to _CodeSignature/CodeResources (like it's supposed to be). Given the error we're seeing, I think this is a red herring, but it may affect behaviour on other OS X versions if they consider that file to be authoritative. * There's a seal (a checksum in the "files" section of _CodeSignature/CodeResources) for defaults/preferences/channel-prefs.js. * There is NO seal for updates.xml Erick reminded me that there's some form of documentation for CodeResources (http://developer.apple.com/library/mac/#technotes/tn2206/_index.html#Omitting%20Files%20from%20the%20Bundle%27s%20Seal), and after looking over that I think we need to include a "weight" for each of our omitted patterns. The docs don't say what happens when two rules tie in weight, but I would bet that the omission loses. The fact that channel-prefs.js (a file shipped with our build) has a seal and updates.xml (which is only created when the first update happens) doesn't supports this theory. I'm currently working on creating test builds in our development environment, because I fear we'll hit additional things we need to exclude still. That's likely to take about 2h.

Encouraging news: the first build I did with the updated CodeResources file doesn't have a seal for channel-prefs.js, and when I locally modified that file, and created updates.xml, it still passed codesign -vv. I'm waiting on the second build of it so that I can test out updates properly.

OK, I've got a working patch now: https://github.com/bhearsum/mozilla-central/commit/e019b9919c6dac7da8a493fae48e15ffd9326347 The only difference between it and the combination of the three previous patches is the addition of the weights. I did two builds in staging of this, including a partial update between them. Before updating, 'codesign -vv FirefoxNightly.app' validated the build. After updating, it remained valid -- despite updates.xml existing, and channel-prefs.js being modified. Separately, I'm testing an additional patch in attempt to fix the broken symlink. I consulted with Ted on it, and I'm trying two different patches right now to see what will happen. As far as I know, this _isn't_ a blocker for this landing, but it'd be great to get sorted out. I'd love for QA to look over the builds a bit, but unfortunately, I can't really make it possible for them to test the updates prior to this re-landing on mozilla-central. I've put the most recent build here: http://people.mozilla.com/~bhearsum/samplebuilds/firefox-15.0a1.en-US.mac.dmg Anyone needing them, please ping me for the username/password.

Okay, I've managed to fix all of the issues. This patch is a roll-up of the 3 previously landed patches plus the additional fixes. Here's a summary of what's changed compared with the previous 3 patches: - Add weights to the omission regexes in the CodeResources file. This fixes the broken signature after update. - Stop listing the Contents/CodeResources file in package-manifest.in, because the packager code doesn't cope with symlinks as we want it to in this case. Instead, I've added a 'ln -sf' in the stage-package target, right after packager runs. Other than that, it's all the same. I've tested partial updates for both en-US and l10n, which worked fine and continued having valid signatures. I've also inspected both builds to make sure that the Contents/CodeResources file is a symlink, as it should be. I'd like to land this ASAP on mozilla-central and respin nightlies so that QA can start testing.

Comment on attachment 626913 [details] [diff] [review] rollup of all patches + fixes Review of attachment 626913 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/installer/packager.mk @@ +770,5 @@ > $(call PACKAGER_COPY, "$(call core_abspath,$(DIST))",\ > "$(call core_abspath,$(DIST)/$(MOZ_PKG_DIR))", \ > "$(MOZ_PKG_MANIFEST)", "$(PKGCP_OS)", 1, 0, 1) > +ifeq (DMG, $(MOZ_PKG_FORMAT)) > + @cd $(DIST)/$(_APPNAME)/Contents && ln -sf _CodeSignature/CodeResources CodeResources I wonder if we need to do this conditionally on having code signing enabled. Will this break other apps by sticking this dangling symlink in there?

I'm not sure...it should be easy to make it conditional on signing, though. I'll throw something at try.

Unfortunately, Contents/_CodeSignature/CodeResources still exists even with this, because I don't know how to ifdef it in package-manifest.in. To the best of my knowledge, this won't break anything. I'm happy to fix that with a bit of guidance, I'd like to get this landed tonight if possible, though.

Comment on attachment 626949 [details] [diff] [review] don't symlink CodeResources when not signing Review of attachment 626949 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/installer/package-manifest.in @@ +23,5 @@ > ; Mac bundle stuff > @APPNAME@/Contents/Info.plist > @APPNAME@/Contents/PkgInfo > @APPNAME@/Contents/Resources/ > +@APPNAME@/Contents/_CodeSignature/CodeResources If you want to #ifdef this, you have to add something to DEFINES in the Makefile: http://mxr.mozilla.org/mozilla-central/source/browser/installer/Makefile.in ::: toolkit/mozapps/installer/packager.mk @@ +556,5 @@ > ifdef MOZ_SIGN_PREPARED_PACKAGE_CMD > +ifeq (Darwin, $(OS_ARCH)) > +MAKE_PACKAGE = $(PREPARE_PACKAGE) \ > + && cd ./$(PKG_DMG_SOURCE) && $(MOZ_SIGN_PREPARED_PACKAGE_CMD) $(MOZ_MACBUNDLE_NAME) && cd $(PACKAGE_BASE_DIR) \ > + && $(INNER_MAKE_PACKAGE) The indentation here is a little weird. Maybe because it's using tabs? Don't use tabs. @@ +770,5 @@ > $(call PACKAGER_COPY, "$(call core_abspath,$(DIST))",\ > "$(call core_abspath,$(DIST)/$(MOZ_PKG_DIR))", \ > "$(MOZ_PKG_MANIFEST)", "$(PKGCP_OS)", 1, 0, 1) > +ifeq (DMG, $(MOZ_PKG_FORMAT)) > +ifneq (,$(filter dmg, $(MOZ_INTERNAL_SIGNING_FORMAT))) Might be better phrased as ifeq(dmg,$(filter dmg,...

Comment on attachment 626949 [details] [diff] [review] don't symlink CodeResources when not signing I addressed the latter two comments: +ifeq (Darwin, $(OS_ARCH)) +MAKE_PACKAGE = $(PREPARE_PACKAGE) \ + && cd ./$(PKG_DMG_SOURCE) && $(MOZ_SIGN_PREPARED_PACKAGE_CMD) $(MOZ_MACBUNDLE_NAME) && cd $(PACKAGE_BASE_DIR) \ + && $(INNER_MAKE_PACKAGE) +else MAKE_PACKAGE = $(PREPARE_PACKAGE) && $(MOZ_SIGN_PREPARED_PACKAGE_CMD) \ $(MOZ_PKG_DIR) && $(INNER_MAKE_PACKAGE) +endif #Darwin + else MAKE_PACKAGE = $(PREPARE_PACKAGE) && $(INNER_MAKE_PACKAGE) endif @@ -762,6 +770,11 @@ endif $(call PACKAGER_COPY, "$(call core_abspath,$(DIST))",\ "$(call core_abspath,$(DIST)/$(MOZ_PKG_DIR))", \ "$(MOZ_PKG_MANIFEST)", "$(PKGCP_OS)", 1, 0, 1) +ifeq (DMG, $(MOZ_PKG_FORMAT)) +ifeq (dmg, $(filter dmg, $(MOZ_INTERNAL_SIGNING_FORMAT))) + @cd $(DIST)/$(_APPNAME)/Contents && ln -sf _CodeSignature/CodeResources CodeResources +endif +endif Per IRC, I'll fix up package-manifest after the fact. I filed bug 758595 for that. Landed in: http://hg.mozilla.org/mozilla-central/rev/caea66e968bf, and I've triggered a new mac nightly.

The first Nightly just finished, here's results from my testing: * Runs fine on 10.8, passes codesign -vv * After updating to it from the previous Nightly, it runs fine on 10.6. codesign -vv says "does not satisfy its designated Requirement". After some poking, I think this is because Contents/CodeResources is missing, because seems to be because it's not in the MAR. I'm guessing that our update generation code doesn't like the symlink. I don't think this a critical issues, given the build still runs. Another Nightly is going now, so we can test signed -> signed updates too.

> codesign -vv says "does not satisfy its designated Requirement" I've seen this on OS X 10.6.8 with builds I signed myself (also on 10.6.8). I suspect it's a spurious error.

I just tested updating from firefox-2012-05-25-03-05-17-mozilla-central (unsigned) to (I think) firefox-2012-05-25-06-29-19-mozilla-central (signed) on OS X 10.8. No problems. I forced an update from About : Nightly : Check For Updates. I got the first nightly to run unsigned by temporarily setting "Allow applications downloaded from" to "Anywhere" before I ran it for the first time. Then I immediately set it back to "Mac App Store and identified developers" before running it again (and performing the update). I'm about to test this again on OS X 10.5.8.

> I'm about to test this again on OS X 10.5.8. Did the same thing on OS X 10.5.8. Again no problems.

Okay, the next Nightly is out. http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-05-25-06-29-19-mozilla-central/firefox-15.0a1.en-US.mac.dmg should get a partial to it, and earlier builds should get a complete. I tested on 10.8 from the aforementioned build -> the new one. It worked fine - I'm updated, and codesign -vv still passes.

I just forced an update from firefox-2012-05-25-06-29-19-mozilla-central (signed) to (I think) firefox-2012-05-25-08-04-55-mozilla-central (signed), on both OS X 10.8 and 10.5.8. Worked fine in both places.

Just tried updating from an old mozilla-central nightly (firefox-2012-01-01-03-10-15-mozilla-central) to whatever's current (probably still firefox-2012-05-25-08-04-55-mozilla-central), on OS X 10.8. I got an odd little visual glitch after I clicked on the restart button -- for a few seconds the distro's icon was crossed out, and nothing seemed to be happening. But then the nightly finished restarting with no problems. We should include updating from older distros in our tests, on the trunk, aurora and beta. (For all I know, the "glitch" I saw may be normal behavior. I almost never update in place -- instead I usually download a new distro.)

(Following up comment #91) There were no relevant messages in the console after this "glitch".

(In reply to Steven Michaud from comment #91) > Just tried updating from an old mozilla-central nightly > (firefox-2012-01-01-03-10-15-mozilla-central) to whatever's current > (probably still firefox-2012-05-25-08-04-55-mozilla-central), on OS X 10.8. > > I got an odd little visual glitch after I clicked on the restart button -- > for a few seconds the distro's icon was crossed out, and nothing seemed to > be happening. But then the nightly finished restarting with no problems. > > We should include updating from older distros in our tests, on the trunk, > aurora and beta. > > (For all I know, the "glitch" I saw may be normal behavior. I almost never > update in place -- instead I usually download a new distro.) I suspect that glitch is because background updates were enabled again today. Probably worth a comment in https://bugzilla.mozilla.org/show_bug.cgi?id=758101

> I suspect that glitch is because background updates were enabled again today. I doubt it. In all my tests I forced an update (in About Nightly : Check for Updates), just a few seconds after starting the nightly. I suspect what I saw was just the normal behavior of the older updater (the one included with the old nightly I updated from). But I know very little about how the updater works (since I never use it), so others who do know will need to comment.

(Following up comment #94) What I called a "glitch" appears to be normal behavior for a full update. I didn't see it forcing an update from FF 11. But I did see it forcing an update from FF 10. Sorry for the confusion :-(

I've been doing a staging release with this patch, and I'm hitting an issue related to MOZ_PKG_PRETTYNAMES paths: 2012-05-25 11:37:43,212 - b31dd2fcfbd4c01a426d28fc71b574cc987cb583: processing Firefox.app.tar.gz on https://mac-signing1.srv.releng.scl3.mozilla.com:9110 2012-05-25 11:37:43,859 - b31dd2fcfbd4c01a426d28fc71b574cc987cb583: uploading for signing 2012-05-25 11:38:18,871 - b31dd2fcfbd4c01a426d28fc71b574cc987cb583: OK 2012-05-25 11:38:18,871 - Copying Firefox.app.tar.gz to cache /builds/slave/rel-m-beta-osx64-bld/signing_cache/dmg/b31dd2fcfbd4c01a426d28fc71b574cc987cb583 mktemp -d mac/en-US/pkg-dmg.50532.XXXXXXXX mktemp: mkdtemp failed on mac/en-US/pkg-dmg.50532.CyYJO3OI: No such file or directory /builds/slave/rel-m-beta-osx64-bld/build/build/package/mac_osx/pkg-dmg: mktemp failed No need to panic about this, it merely needs to be fixed before we start Beta 6 on Tuesday. I'm looking into it right now.

A similar mktemp command runs in a non-signing release build: mktemp -d mac/en-US/pkg-dmg.76062.XXXXXXXX Which makes me think that we're in the wrong directory after running $(MOZ_SIGN_PREPARED_PACKAGE_CMD).

D'oh, it looks like I missed a hunk of the packager.mk patch when I applied this to my mozilla-beta repository, so this may end up being a non-issue. More to come in ~1h when my rebuild finishes.

(In reply to Ben Hearsum [:bhearsum] from comment #98) > D'oh, it looks like I missed a hunk of the packager.mk patch when I applied > this to my mozilla-beta repository, so this may end up being a non-issue. > More to come in ~1h when my rebuild finishes. Forgot to update this bug yesterday: it turned out to be an issue with my partial landing of the patch. Nothing to worry about here.

I got the clear from Alex yesterday to land this on Aurora today. I forgot to set the flag, but I've landed anyways because the coverage is important.

Just did the following tests on Aurora: * Downloaded http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/05/2012-05-27-03-31-43-mozilla-aurora/firefox-14.0a2.en-US.mac.dmg * Copied it to the Desktop * Ran 'codesign -vv FirefoxAurora.app', which declared the build valid * Double-clicked to run it. Had to click through the normal "You downloaded this file from the Internet" dialog, the build ran after that. Went to About -> Aurora to trigger an update check, got a partial update. * Applied the update, no problems. Build ran again automatically afterwards. * Ran 'codesign -vv' again, build is still valid. There's currently an Aurora nightly build running. I'm going to wait for it to finish and as long as everything looks fine after I update to it I'll go ahead and land on Beta.

I updated fine to the next build. Landed on mozilla-beta: https://hg.mozilla.org/releases/mozilla-beta/rev/fa89a1186a7c We actually need to land this on esr10, too - I almost forgot.

Comment on attachment 627526 [details] [diff] [review] patch w/ comment #84 applied The last hunk of packager.mk didn't apply because the context was very different, but there's no functional change to the patch so I can land on esr10 without issue. I can't test on try, because there's no way to do these builds anymore there.

We're not sure whether or not we're going to land on esr10. We're on all the other important branches, calling this fixed. bugs 758644 and 758595 are tracking minor follow-ups. Thank again to everyone that helped out here.

Just did some tests. Most turned out fine, but I found a big problem -- today's Firefox debug nightly (firefox-2012-05-29-mozilla-beta-debug) is signed with "Mozilla Fake DMG Cert"! This needs to be fixed!

That's expected. Debug "nightlies" are simply a on-change debug build that gets copied into that directory. We only sign builds that we have a userbase on with real certificates. This means: Nightly (mozilla-central, as well as a few project branches that have nightlies enabled), Aurora, Beta, and Release.

Today's mozilla-central and aurora nightlies are fine (correctly signed). So are yesterday's. And yesterday's mozilla-central and aurora nightlies both ran successfully after updating. The mozilla-central update was a full signed-to-signed update -- the first time I'd run one of those. Glad there were no problems.

> That's expected. Oh, OK. > We only sign builds that we have a userbase on with real certificates. I'm not so sure that's a good idea. It certainly gave me a big scare!

(In reply to Steven Michaud from comment #108) > > That's expected. > > Oh, OK. > > > We only sign builds that we have a userbase on with real certificates. > > I'm not so sure that's a good idea. It certainly gave me a big scare! I can certainly see the argument, but I don't really think it's a big deal when the target audience is developers. Infrasec told us they didn't want anything shipped with real certs that didn't a) live on a Level 3 branch (http://www.mozilla.org/hacking/commit-access-policy/), and b) had users on it.

I've opened bug 759467.

I just tested the latest RC for FF 13.0b6 on OS X 10.8, and had no problems. It's signed with a "real" cert.

I use 'Keychain Services Integration' extension and that's one of the reasons I'm interested in having Firefox having signed builds, so it doesn't keep asking me to allow access to the Keychain. Just updated to 13.0b6. Still asks for Keychain access, multiple times. Checking the signature: $ codesign -vvvv /Applications/Firefox.app /Applications/Firefox.app: valid on disk /Applications/Firefox.app: does not satisfy its designated Requirement

(In reply to spinifer from comment #112) > I use 'Keychain Services Integration' extension and that's one of the > reasons I'm interested in having Firefox having signed builds, so it doesn't > keep asking me to allow access to the Keychain. > > Just updated to 13.0b6. Still asks for Keychain access, multiple times. > > Checking the signature: > > $ codesign -vvvv /Applications/Firefox.app > /Applications/Firefox.app: valid on disk > /Applications/Firefox.app: does not satisfy its designated Requirement That codesign output surprises me greatly. Would it be possible for you to tar/zip up your Firefox.app directory and send it to me or post it somewhere?

Sure, I'll mail you a link to the file on my Google Drive. I guess some extension must be writing stuff where it shouldn't? I should add that in the meantime I deleted Firefox.app, downloaded a fresh new 13.0b6, and installed it. Still the same codesign output, and same annoying allow access messages.

(In reply to comment #112) > /Applications/Firefox.app: does not satisfy its designated Requirement I also see this on OS X 10.5 and 10.6, but not on 10.7 or 10.8. I'm pretty sure that means it's a spurious error message, and unrelated to your problem. Please open a new bug on problems with the "Keychain Services Integration" extension. When you do, please be sure to provide detailed information about what versions(s) of this extension you've tested, which version(s) of Firefox, and on which versions of OS X.

(In reply to Steven Michaud from comment #115) I'll be happy to after Ben investigates the compressed file I sent him. But knowing that the Mac Keychain relies on the application signing to verify it's the same app accessing the Keychain, that it hasn't been changed, makes me still think this is a signing problem.

(In reply to spinifer from comment #116) > (In reply to Steven Michaud from comment #115) > > I'll be happy to after Ben investigates the compressed file I sent him. > > But knowing that the Mac Keychain relies on the application signing to > verify it's the same app accessing the Keychain, that it hasn't been > changed, makes me still think this is a signing problem. I asked Steven to help me with that, and neither of us could figure out exactly why it's still not valid. Unfortunately, I think this means that you're stuck in that situation for the time being -- we're not planning to fix 10.5 through 10.7 specific problems with signing at the moment -- our goal is merely to make sure Firefox runs everywhere. Sorry :(

But please do open that bug, with *lots* of information. Like I said, I think your problem is completely unrelated to the error message, and there *may* turn out to be something we can do about it. But I'll tell you in advance that we've got lots of other, more urgent problems. So we might not get to your bug for a while.

Comment on attachment 627526 [details] [diff] [review] patch w/ comment #84 applied [Triage Comment] Approving for ESR, looks like it will be obvious if builds are affected when this lands so you'll know if anything stops working here.

I hate to insist on this here before opening a different bug, but I've been trying to track down the problem with Keychain Access on the Keychain Services Integration issue tracker, and there are evidences pointing to this being a build signing issue between OSX 10.6 and 10.7. Here's the issue: http://code.google.com/p/mozilla-keychain/issues/detail?id=48 After some testing on OS X 10.6 and 10.7 there are some preliminary conclusions on 'Designated Requirements' (it's how the system tracks if it's the same app), and why the message that it does not satisfy it's designated requirements is important. Here's an explanation of this problem: http://www.red-sweater.com/blog/2390/developer-id-gotcha "So, I’m not 100% fact based yet, but I think the new moral of the story is: if you need to run earlier than 10.7, specify your DR literally because the default DR generated for a Developer ID certificiate will not work perfectly on 10.6 and earlier."

I opened a new bug considering the possibility of a security risk (the build might be signed by others than Mozilla for version prior to OSX 10.7) . https://bugzilla.mozilla.org/show_bug.cgi?id=765271

Comment on attachment 627526 [details] [diff] [review] patch w/ comment #84 applied removing this for now to reduce any confusion. when we make the call on esr signing this can be approved again if needed.

Comment on attachment 627526 [details] [diff] [review] patch w/ comment #84 applied Waiting for ESR17 to roll this out.