723176 - support mac dmg signing in the build system

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[[e-mail] (:bhearsum)]

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!

Comment 1

[[e-mail] (:bhearsum)]

Attachment: sample code resources file

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.

Comment 2

[(not currently active) Ted Mielczarek]

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

Comment 3

[[e-mail] (:bhearsum)]

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

Comment 4

[Erick Dransch [:edransch]]

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.

Comment 5

[Erick Dransch [:edransch]]

Attachment: Condensed CodeResources file

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

Comment 6

[Erick Dransch [:edransch]]

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.

Comment 7

[[e-mail] (:bhearsum)]

(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.

Comment 8

[[e-mail] (:bhearsum)]

(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?

Comment 9

[(not currently active) Ted Mielczarek]

I have no idea what that is.

Comment 10

[[e-mail] (:bhearsum)]

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.

Comment 11

[Erick Dransch [:edransch]]

Attachment: patch to buildsystem

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

Comment 12

[[e-mail] (:bhearsum)]

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.

Comment 13

[Erick Dransch [:edransch]]

Attachment: Feb. 17 Nightly Signed CodeResources

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 14

[(not currently active) Ted Mielczarek]

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.

Comment 15

[(not currently active) Ted Mielczarek]

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

Comment 16

[[e-mail] (:bhearsum)]

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.

Comment 17

[Erick Dransch [:edransch]]

Attachment: Add MOZ_INTERNAL_SIGNING_FORMAT for Darwin builds

Comment 18

[Erick Dransch [:edransch]]

Attachment: Add MOZ_INTERNAL_SIGNING_FORMAT for Darwin builds

Previous patch was the wrong file.

Comment 19

[[e-mail] (:bhearsum)]

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.

Comment 20

[Erick Dransch [:edransch]]

<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>

Comment 21

[[e-mail] (:bhearsum)]

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 ?

Comment 22

[Erick Dransch [:edransch]]

Attachment: m-c patch to integrate mac signing

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

Comment 23

[[e-mail] (:bhearsum)]

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.

Comment 24

[Mozilla RelEng Bot]

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:

Comment 25

[Erick Dransch [:edransch]]

Attachment: m-c patch to integrate mac signing

Updated diff to avoid bitrot

Comment 26

[Mozilla RelEng Bot]

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

Comment 27

[Mozilla RelEng Bot]

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/[e-mail]-cbb75a3e52f2

Comment 28

[[e-mail] (:bhearsum)]

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 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

302 ted

Comment 30

[[e-mail] (:bhearsum)]

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.

Comment 31

[Erick Dransch [:edransch]]

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?

Comment 32

[Erick Dransch [:edransch]]

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?

Comment 33

[Erick Dransch [:edransch]]

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

Comment 34

[(not currently active) Ted Mielczarek]

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

Comment 35

[Erick Dransch [:edransch]]

Attachment: Patch to build-system

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

Comment 36

[[e-mail] (:bhearsum)]

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 37

[(not currently active) Ted Mielczarek]

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.

Comment 38

[[e-mail] (:bhearsum)]

(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.

Comment 39

[Erick Dransch [:edransch]]

Attachment: Patch to build-system

Address Ted's review.

Comment 40

[[e-mail] (:bhearsum)]

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.

Comment 41

[[e-mail] (:bhearsum)]

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

Comment 42

[[e-mail] (:bhearsum)]

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 43

[[e-mail] (:bhearsum)]

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 44

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 45

[[e-mail] (:bhearsum)]

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.

Comment 46

[[e-mail] (:bhearsum)]

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

Comment 47

[Steven Michaud [:smichaud] (Retired)]

Is this an error on the signing server?

Comment 48

[Steven Michaud [:smichaud] (Retired)]

> 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.

Comment 49

[[e-mail] (:bhearsum)]

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...

Comment 50

[[e-mail] (:bhearsum)]

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 51

[[e-mail] (:bhearsum)]

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

Comment 52

[Mark Banner (:standard8)]

(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.

Comment 53

[[e-mail] (:bhearsum)]

(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

Comment 54

[[e-mail] (:bhearsum)]

(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.

Comment 55

[[e-mail] (:bhearsum)]

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.

Comment 56

[[e-mail] (:bhearsum)]

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

Comment 57

[[e-mail] (:bhearsum)]

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.

Comment 58

[Steven Michaud [:smichaud] (Retired)]

(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' :-)

Comment 59

[[e-mail] (:bhearsum)]

(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.

Comment 60

[[e-mail] (:bhearsum)]

Attachment: add PACKAGE_BASE_DIR to packager.mk, override in l10n.mk, to fix mac repacks

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 61

[[e-mail] (:bhearsum)]

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.

Comment 62

[[e-mail] (:bhearsum)]

This seems to have fixed l10n.

Comment 63

[[e-mail] (:bhearsum)]

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

Comment 64

[Alex Keybl [:akeybl]]

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.

Comment 65

[Steven Michaud [:smichaud] (Retired)]

> 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?).

Comment 66

[[e-mail] (:bhearsum)]

(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.

Comment 67

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 68

[[e-mail] (:bhearsum)]

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).

Comment 69

[Steven Michaud [:smichaud] (Retired)]

Oh, OK.  Thanks!  Big sigh of relief!! :-)

I suppose I should go find my Hitchhiker's Guide.

Comment 71

[[e-mail] (:bhearsum)]

Attachment: add omission for removed-files

Comment 72

[[e-mail] (:bhearsum)]

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.

Comment 73

[[e-mail] (:bhearsum)]

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 74

[[e-mail] (:bhearsum)]

Comment on attachment 611452 [details] [diff] [review]
Patch to build-system

I'm going to combine these 3 patches into a single roll-up.

Comment 75

[[e-mail] (:bhearsum)]

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!

Comment 76

[[e-mail] (:bhearsum)]

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.

Comment 77

[[e-mail] (:bhearsum)]

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.

Comment 78

[[e-mail] (:bhearsum)]

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.

Comment 79

[[e-mail] (:bhearsum)]

Attachment: rollup of all patches + fixes

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 80

[(not currently active) Ted Mielczarek]

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?

Comment 81

[[e-mail] (:bhearsum)]

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

Comment 82

[[e-mail] (:bhearsum)]

Attachment: don't symlink CodeResources when not signing

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 83

[(not currently active) Ted Mielczarek]

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 84

[[e-mail] (:bhearsum)]

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.

Comment 85

[[e-mail] (:bhearsum)]

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.

Comment 86

[Steven Michaud [:smichaud] (Retired)]

> 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.

Comment 87

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 88

[Steven Michaud [:smichaud] (Retired)]

> 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.

Comment 89

[[e-mail] (:bhearsum)]

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.

Comment 90

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 91

[Steven Michaud [:smichaud] (Retired)]

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.)

Comment 92

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #91)

There were no relevant messages in the console after this "glitch".

Comment 93

[[e-mail] (:bhearsum)]

(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

Comment 94

[Steven Michaud [:smichaud] (Retired)]

> 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.

Comment 95

[Steven Michaud [:smichaud] (Retired)]

(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 :-(

Comment 96

[[e-mail] (:bhearsum)]

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.

Comment 97

[[e-mail] (:bhearsum)]

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).

Comment 98

[[e-mail] (:bhearsum)]

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.

Comment 99

[[e-mail] (:bhearsum)]

(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.

Comment 100

[[e-mail] (:bhearsum)]

Attachment: patch w/ comment #84 applied

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.

Comment 101

[[e-mail] (:bhearsum)]

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.

Comment 102

[[e-mail] (:bhearsum)]

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 103

[[e-mail] (:bhearsum)]

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.

Comment 104

[[e-mail] (:bhearsum)]

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.

Comment 105

[Steven Michaud [:smichaud] (Retired)]

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!

Comment 106

[[e-mail] (:bhearsum)]

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.

Comment 107

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 108

[Steven Michaud [:smichaud] (Retired)]

> 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!

Comment 109

[[e-mail] (:bhearsum)]

(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.

Comment 110

[Steven Michaud [:smichaud] (Retired)]

I've opened bug 759467.

Comment 111

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 112

[spinifer]

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

Comment 113

[[e-mail] (:bhearsum)]

(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?

Comment 114

[spinifer]

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.

Comment 115

[Steven Michaud [:smichaud] (Retired)]

(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.

Comment 116

[spinifer]

(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.

Comment 117

[[e-mail] (:bhearsum)]

(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 :(

Comment 118

[Steven Michaud [:smichaud] (Retired)]

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 119

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 120

[spinifer]

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."

Comment 121

[spinifer]

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 122

[Lukas Blakk [:lsblakk] use ?needinfo]

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 123

[Alex Keybl [:akeybl]]

Comment on attachment 627526 [details] [diff] [review]
patch w/ comment #84 applied

Waiting for ESR17 to roll this out.