u49640 Reporter
	Description • 17 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7

According to the link, Mac OS X understands signed applications. Since the Windows firefox.exe is already signed, it would make sense to sing firefox.app too (and thunderbird.app, but I'm to lazy to file another bug)

From the URL:
"Signed Applications

Feel safe with your applications. A digital signature on an application verifies its identity and ensures its integrity. All applications shipped with Leopard are signed by Apple, and third-party software developers can also sign their applications."

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

	Marcia Knous [:marcia]
	Comment 1 • 17 years ago

Let me know if a separate bugs is required for Tbird.

	Nick Thomas [:nthomas] (UTC+12)
	Comment 2 • 17 years ago

Some documentation from Apple:

http://developer.apple.com/documentation/Security/Conceptual/CodeSigningGuide/index.html#//apple_ref/doc/uid/TP40005929

	Mark Mentovai
	Comment 3 • 17 years ago

You need to be on 10.5 to do this in the supported way, and all of the release tinderbuilders are 10.4.

I don't know if there's any unsupported way to do this on 10.4, I haven't looked closely.

	Marcia Knous [:marcia]
	Comment 4 • 17 years ago

This would be a nice polish bug to get for final. I can't raise the visibility since the flags don't pertain to this component.

	Mark Mentovai
	Comment 5 • 17 years ago

Do you want this to be done automatically from within tinderbox, or do you want for there to be a separate procedure to do it?  From what I understand, signing the Windows builds is done separately, outside of the ordinary tinderbox build.

	Mark Mentovai
	Comment 6 • 17 years ago

This could be supported using something similar to what I crafted in bug 409459.  The logistical (on-tinderbox-or-elsewhere) and political (get-the-certificate) questions still need to be answered.

	Marcia Knous [:marcia]
	Comment 7 • 17 years ago

Mark: I guess the answer to comment 5 would be up the build team. If there were an easy way to do it so we could get the signed bits without waiting for a separate build, I am all for it. Thanks for helping out with this.

	Mark Mentovai
	Comment 8 • 17 years ago

If you wanted to do this on a tinderbox, the tinderbox would need to be running Leopard.  Currently, there are no Leopard tinderboxes anywhere in all of Mozilla-land, except for the one we set up for Camino on the community build network.

Given a Leopard tinderbox, someone would still need to make a policy decision about whether you want to sign all builds always (meaning that the tinderbox doing the signed builds would need the key installed on it) or only releases that have been through some QA (meaning that the key could be guarded more closely).  I'm not sure if there are existing policy reasons for signing the Windows builds the way you do them, so barking up that tree might be a good first step.  If nobody's opposed to doing them automated, then we can start thinking about tinderboxen or other things, and we can get the script polished up for use by other apps than Camino.

We'll also want to talk about certificates as they relate to Camino, so if you're interested, drop on by bug 409459.

	John O'Duinn [:joduinn] (please use "needinfo?" flag)
	Comment 9 • 17 years ago

(In reply to comment #3)
> You need to be on 10.5 to do this in the supported way, and all of the release
> tinderbuilders are 10.4.
> I don't know if there's any unsupported way to do this on 10.4, I haven't
> looked closely.

For the FF3.0 releases, we're still on 10.4, so looks like earliest we can consider doing this is in moz2. Reassigning to "future".

	Nomis101
	Comment 10 • 16 years ago

Now there are 10.5 tinderbuilders, for Firefox 3.1 and Firefox 3.2. Are there any planes now to sign the Mac-Binary for OSX 10.5?

	bhearsum@mozilla.com (:bhearsum)
	Comment 11 • 16 years ago

Not at this time. I can't see this happening until at least the summer, and probably not until later than that.

	Daniel Veditz [:dveditz]
	Comment 12 • 16 years ago

I don't think this should be done on the tinderbuilders, the insecurity of that setup would defeat the purpose. Or we could sign those with a cheap cert for nightlies, but I don't see that buying us much. The releases need to be signed on a restricted build signing machine with controlled access to the key.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 13 • 15 years ago

We need to raise the priority of this, I think.  There is currently no way for a user to know if they got an intact Mac Firefox from us, since we deliver over HTTP and there's no signature.  (Like, security experts at Black Hat couldn't tell and were nervous about it, and if *they* can't tell...)

We don't want to do it on the tinderbuilders, we should do it like we do it for Windows (in which case we don't have to upgrade the tinderboxes either, if that were still needed, since we could just sign on a 10.5 machine I think).

Can we get an updated ETA here, or an idea of how someone else could get it working?

	Chris AtLee [:catlee]
	Comment 14 • 15 years ago

(In reply to comment #13)
> We need to raise the priority of this, I think.  There is currently no way for
> a user to know if they got an intact Mac Firefox from us, since we deliver over
> HTTP and there's no signature.  (Like, security experts at Black Hat couldn't
> tell and were nervous about it, and if *they* can't tell...)

We do GPG sign all our installers, and the detached signatures are available on our ftp site.  I know, terrible user experience, but it's _possible_ to verify that you downloaded an intact build.

	Chris AtLee [:catlee]
	Comment 15 • 15 years ago

From the apple docs it looks like we may be able to re-use our existing signing key, which would be nice.

I'd want a separate signing machine to do this on, like we have for windows.

	Chris Cooper [:coop] (he/him)
	Comment 16 • 15 years ago

Mass move of bugs from Release Engineering:Future -> Release Engineering. See
http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.

	Markus Dommann
	Comment 17 • 15 years ago

As long as the Mac binaries are not code signed, addons like mozilla-keychain cannot work 100% satisfactorily.

Because the Firefox binary is not code signed, access to each password of Keychain has to be confirmed everytime a new version of Firefox is installed. The same applies to Thunderbird (I found no bug entry) and (I guess) Camino (Bug 400296).

I already voted for the bug but wanted to point out that it would be a real gain for Mac users.

	Mike Taylor [:bear]
	Comment 18 • 15 years ago

We should be able to use the codesign tool that Apple provides to use any normal signing certificate (which would be imported into the Keystore) to sign the binary.

This would need to be verified (i'm working from older notes and memory) and it also requires an OS X signing computer.  Another glitch that I'm remembering is that the codesign binary seemed to require being run from a windowed session, i.e. you couldn't run it from a headless ssh session.

	Mike Taylor [:bear]
	Comment 19 • 15 years ago

Morphing this bug to be a generic OS X signing issue as the solution is the same for 10.5 and 10.6

	LegNeato
	Comment 20 • 15 years ago

I doubt it requires a windowed session, as Apple's own RelEng signing system is
headless. Perhaps they use a private framework though.

I am more concerned about breaking the signature with updates (manual and
auto), changing files, localization, etc. Once we sign it the assumption is a
broken signature is "bad". It would also be arguably worse to have something
like mozilla-keychain work well and then after an update have it start to ask
for permission to access the keychain.

Some considerations:
* We'd have to ignore any dynamically changing files (blocklists and such, if
stored in the app bundle) in the code signature. This is fairly trivial to do
once we identify the files
* We can have "optionally" signed files. That is, if the file isn't signed it
is a failure, but if it is signed it needs to match the signature provided. Not
sure which files would fall in this category for Firefox, perhaps
localizations?
* If files removed between versions will be left over in the app bundle, it
will cause codesign -v to fail with "seal broken / extra files" messages and
the prompts will start

	Chris AtLee [:catlee]
	Comment 21 • 15 years ago

(In reply to comment #20)
> I doubt it requires a windowed session, as Apple's own RelEng signing system is
> headless. Perhaps they use a private framework though.
> 
> I am more concerned about breaking the signature with updates (manual and
> auto), changing files, localization, etc. Once we sign it the assumption is a
> broken signature is "bad". It would also be arguably worse to have something
> like mozilla-keychain work well and then after an update have it start to ask
> for permission to access the keychain.
> 
> Some considerations:
> * We'd have to ignore any dynamically changing files (blocklists and such, if
> stored in the app bundle) in the code signature. This is fairly trivial to do
> once we identify the files
> * We can have "optionally" signed files. That is, if the file isn't signed it
> is a failure, but if it is signed it needs to match the signature provided. Not
> sure which files would fall in this category for Firefox, perhaps
> localizations?
> * If files removed between versions will be left over in the app bundle, it
> will cause codesign -v to fail with "seal broken / extra files" messages and
> the prompts will start

How is any of this different than on windows?

	LegNeato
	Comment 22 • 15 years ago

Not sure if it is any different. I'm just saying these were considerations when shipping updates to signed bundles on Mac OS X previously and we should at least be aware.

	Mike Taylor [:bear]
	Comment 23 • 15 years ago

(In reply to comment #20)
> I doubt it requires a windowed session, as Apple's own RelEng signing system is
> headless. Perhaps they use a private framework though.

It was something I remembered from setting up Seesmic's build/release scripts and wanted to mark it for my research :)

	Mike Taylor [:bear]
	Comment 24 • 15 years ago

(In reply to comment #23)
> (In reply to comment #20)
> > I doubt it requires a windowed session, as Apple's own RelEng signing system is
> > headless. Perhaps they use a private framework though.
> 
> It was something I remembered from setting up Seesmic's build/release scripts
> and wanted to mark it for my research :)

For completeness - here is how Apple probably solved it:

http://lists.apple.com/archives/apple-cdsa/2008/Jan/msg00027.html

	Syed Albiz [:salbiz]
	Comment 25 • 14 years ago

rough patch to add support for mac signing, tested on a mac slave with a manually unlocked keychain and self-signed certificate set up for testing. A few things here are obviously going to need work/testing (hardcoding of app bundle paths and single-file mac signing for updates are the main ones that come to mind).

	Chris AtLee [:catlee]
	Comment 26 • 14 years ago

Comment on attachment 488135 [details] [diff] [review]
integrate mac signing into signing scripts

A great start!

>diff --git a/release/signing/sign-release.py b/release/signing/sign-release.py
>--- a/release/signing/sign-release.py
>+++ b/release/signing/sign-release.py
>@@ -1,28 +1,68 @@
> #!/usr/bin/python
> import tempfile, os, shutil, time, bz2, sys, re
> import logging
> from subprocess import *
> 
> from signing import *
> 
> log = logging.getLogger()
>+def signbundle(dirname, keydir, fake=False):
>+    """Sign a packaged app bundle with the keychain in keydir"""
>+    if fake:
>+        time.sleep(1)
>+        return
>+    #get top level app bundles in the package dir
>+    bundles = os.walk(dirname).next()
>+    for bundle in bundles[1]:

This is a little hard to understand.  bundles = [d for d in os.listdir(dirname) if os.path.isdir(dirname)] might be better?

>-def signfile(filename, keydir, fake=False):
>+def signmacfile(filename, keydir, platform):
>+    basename = os.path.basename(filename)
>+    dirname = os.path.dirname(filename)
>+    stdout = tempfile.TemporaryFile()
>+    command = ['codesign',
>+        '-s', 'cltsign',
>+        '--keychain', '%s/cltsign.keychain' % keydir,
>+        '-i', 'org.mozilla.firefox',
>+        basename]
>+    try:
>+        check_call(command, cwd=dirname, stdout=stdout, stderr=STDOUT)
>+    except:
>+        stdout.seek(0)
>+        data = stdout.read()
>+        log.exception(data)
>+        raise
>+
>+def signfile(filename, keydir, platform='win32', fake=False):
>     """Sign the given file with keys in keydir.
> 
>     If fake is True, then don't actually sign anything, just sleep for a
>     second to simulate signing time."""
>     if fake:
>         time.sleep(1)
>         return
>+    if platform == 'mac':
>+        return signmacfile(filename, keydir, platform)

Can you split off the rest into a 'signwinfile' function?

>@@ -152,69 +192,72 @@ class Signer:
>         nFiles = 0
>         cacheHits = 0
>         nSigned = 0
>         tmpdir = tempfile.mkdtemp()
>         try:
>             # Unpack it
>             logs.append("Unpacking %s to %s" % (pkgfile, tmpdir))
>             unpackfile(pkgfile, tmpdir)
>-            # Swap in files we have already signed
>-            for f in findfiles(tmpdir):
>-                # We don't need to do anything to files we're not going to sign
>-                if not shouldSign(f):
>-                    continue
>+            if pkgfile.endswith('.dmg'):
>+                signbundle(tmpdir, self.keydir, self.fake)

Putting a 'continue' or 'return' here would save having to have this giant 'else' clause below.

>+            else:
>+                # Swap in files we have already signed
>+                for f in findfiles(tmpdir):
>+                    # We don't need to do anything to files we're not going to sign
>+                    if not shouldSign(f):
>+                        continue
> 
>-                h = sha1sum(f)
>-                basename = os.path.basename(f)
>-                nFiles += 1
>+                    h = sha1sum(f)
>+                    basename = os.path.basename(f)
>+                    nFiles += 1
> 
>-                # Look in the cache for another file with the same original hash
>-                cachedFile = self.getFile(h, f)
>-                if cachedFile:
>-                    cacheHits += 1
>-                    assert os.path.basename(cachedFile) == basename
>-                    logs.append("Copying %s from %s" % (basename, cachedFile))
>-                    # Preserve the original file's mode; don't use the cached mode
>-                    # We usually process installer .exe's first, and 7z doesn't
>-                    # preserve the file mode, so the cached copies of the files
>-                    # are mode 0666.  In the mar files, executables have mode
>-                    # 0777, so we want to preserve that.
>-                    copyfile(cachedFile, f, copymode=False)
>-                else:
>-                    # We need to sign this file
>-                    # If this file is compressed, check if we have a cached copy that
>-                    # is uncompressed
>-                    if compressed:
>-                        bunzip2(f)
>-                        h2 = sha1sum(f)
>-                        cachedFile = self.getFile(h2, f)
>-                        if cachedFile:
>-                            # We have a cached copy of this file that is uncompressed.
>-                            # So copy it into our dstdir, and recompress it, and
>-                            # save it for future use.
>-                            cacheHits += 1
>-                            assert os.path.basename(cachedFile) == basename
>-                            logs.append("Copying %s from uncompressed %s" % (basename, cachedFile))
>-                            # See note above about not copying the file's mode
>-                            copyfile(cachedFile, f, copymode=False)
>+                    # Look in the cache for another file with the same original hash
>+                    cachedFile = self.getFile(h, f)
>+                    if cachedFile:
>+                        cacheHits += 1
>+                        assert os.path.basename(cachedFile) == basename
>+                        logs.append("Copying %s from %s" % (basename, cachedFile))
>+                        # Preserve the original file's mode; don't use the cached mode
>+                        # We usually process installer .exe's first, and 7z doesn't
>+                        # preserve the file mode, so the cached copies of the files
>+                        # are mode 0666.  In the mar files, executables have mode
>+                        # 0777, so we want to preserve that.
>+                        copyfile(cachedFile, f, copymode=False)
>+                    else:
>+                        # We need to sign this file
>+                        # If this file is compressed, check if we have a cached copy that
>+                        # is uncompressed
>+                        if compressed:
>+                            bunzip2(f)
>+                            h2 = sha1sum(f)
>+                            cachedFile = self.getFile(h2, f)
>+                            if cachedFile:
>+                                # We have a cached copy of this file that is uncompressed.
>+                                # So copy it into our dstdir, and recompress it, and
>+                                # save it for future use.
>+                                cacheHits += 1
>+                                assert os.path.basename(cachedFile) == basename
>+                                logs.append("Copying %s from uncompressed %s" % (basename, cachedFile))
>+                                # See note above about not copying the file's mode
>+                                copyfile(cachedFile, f, copymode=False)
>+                                bzip2(f)
>+                                if remember:
>+                                    logs.append("Caching compressed %s as %s" % (f, h))
>+                                    self.rememberFile(h, f)
>+                                continue
>+
>+                        nSigned += 1
>+                        logs.append("Signing %s" % f)
>+                        signfile(f, self.keydir, self.fake)

Do you need to pass platform in here?

>+def unpackdmg(dmgfile, destdir):
>+    """Unpack the given dmgfile into destdir, using hdiutil"""
>+    nullfd = open(os.devnull, "w")
>+    dmgfile = os.path.abspath(dmgfile)
>+    unpack_dir = tempfile.mkdtemp()
>+    try:
>+        check_call(['hdiutil', 'attach', dmgfile,
>+                    '-readonly', '-mountroot', unpack_dir], stdout=nullfd)
>+        check_call(['rsync', '-av', unpack_dir, destdir], stdout=nullfd)
>+    except:
>+        log.exception("Error unpacking dmg %s to %s", dmgfile, destdir)
>+        raise
>+    nullfd.close()
>+    try:
>+        check_call(['hdiutil', 'detach', os.path.join(unpack_dir, 'Firefox')])
>+    except:
>+        log.exception("Error cleaning up %s", unpack_dir)
>+        raise

Take a look at our installdmg.sh script for some extra error checking you may want to do here.  In fact, we may want to use installdmg.sh.  The problem is that hdiutil attach can return before the disk is fully attached.

	bhearsum@mozilla.com (:bhearsum)
	Comment 27 • 14 years ago

Comment on attachment 488135 [details] [diff] [review]
integrate mac signing into signing scripts

>diff --git a/release/signing/sign-release.py b/release/signing/sign-release.py

>+def signbundle(dirname, keydir, fake=False):
>+    """Sign a packaged app bundle with the keychain in keydir"""
>+    if fake:
>+        time.sleep(1)
>+        return
>+    #get top level app bundles in the package dir
>+    bundles = os.walk(dirname).next()
>+    for bundle in bundles[1]:
>+        stdout = tempfile.TemporaryFile()
>+        command = ['codesign',
>+            '-s', 'cltsign',

Can you define this at the top of a file somewhere? Bonus points if it's overridable.

>+            '--keychain', '%s/cltsign.keychain' % keydir,

Same for the cltsign.keychain portion of this.

>+            bundle]
>+        try:
>+            check_call(command, cwd=dirname, stdout=stdout, stderr=STDOUT)
>+        except:
>+            stdout.seek(0)
>+            data = stdout.read()
>+            log.exception(data)
>+            raise
> 
>-def signfile(filename, keydir, fake=False):
>+def signmacfile(filename, keydir, platform):
>+    basename = os.path.basename(filename)
>+    dirname = os.path.dirname(filename)
>+    stdout = tempfile.TemporaryFile()
>+    command = ['codesign',
>+        '-s', 'cltsign',
>+        '--keychain', '%s/cltsign.keychain' % keydir,
>+        '-i', 'org.mozilla.firefox',

This too, hardcoding 'firefox' makes it hard to re-use this work for other projects.




>+def unpackdmg(dmgfile, destdir):
>+    """Unpack the given dmgfile into destdir, using hdiutil"""
>+    nullfd = open(os.devnull, "w")
>+    dmgfile = os.path.abspath(dmgfile)
>+    unpack_dir = tempfile.mkdtemp()
>+    try:
>+        check_call(['hdiutil', 'attach', dmgfile,
>+                    '-readonly', '-mountroot', unpack_dir], stdout=nullfd)
>+        check_call(['rsync', '-av', unpack_dir, destdir], stdout=nullfd)
>+    except:
>+        log.exception("Error unpacking dmg %s to %s", dmgfile, destdir)
>+        raise
>+    nullfd.close()
>+    try:
>+        check_call(['hdiutil', 'detach', os.path.join(unpack_dir, 'Firefox')])
>+    except:
>+        log.exception("Error cleaning up %s", unpack_dir)
>+        raise

I agree with Catlee's suggestion of using the existing unpackdmg script.

>+def packdmg(dmgfile, srcdir):
>+    """Recreate the files in srcdir into the given dmgfile"""
>+    command = ['hdiutil',
>+        'makehybrid',
>+        '-hfs',
>+        '-hfs-volume-name', dmgfile.rstrip(".dmg"),
>+        '-o', dmgfile,
>+        srcdir]
>+    try:
>+       check_call(command)
>+    except:
>+        log.exception("Error packing %s into %s", os.path.abspath(srcdir), os.path.abspath(dmgfile))
>+        raise

This part concerns me a little bit, we do a lot of extra when we package the original DMG. The wrapper script that the build system uses sets the volume name, a background, a disk image icon, inserts a symlink, and possibly other things. Have you compared the unsigned vs. signed to see if that information is retained?

At the very least we'll need to make sure we duplicate that. I'm a bit worried about the invocation in the build system changing, and us missing that, or different invocations between branches. I'm not quite sure how to solve that last part without requiring a full sourceRepo clone. We can live with just duplicating the existing logic for now, I guess.

	Syed Albiz [:salbiz]
	Comment 28 • 14 years ago

Should take care of most of the issues brought up. I have verified this script on the en-US dmg, but will obviously need to do a full run-through comparing the repacked dmg to the original unsigned dmg.

In addition to changing the dmg unpacking, I refactored how bundles are signed, so that we don't miss sub-bundles and individual binaries that live under MacOS/, which don't get automatically signed by signing the bundle.

	bhearsum@mozilla.com (:bhearsum)
	Comment 29 • 14 years ago

Comment on attachment 488989 [details] [diff] [review]
refine dmg packing/unpacking

>diff --git a/buildfarm/utils/installdmg.sh b/buildfarm/utils/installdmg.sh

> # Now we can copy everything out of the mnt directory into the current directory
>-rsync -av ./mnt/* .
>+rsync -av ./mnt/ .

Any particular reason you're changing this?


>diff --git a/release/signing/Makefile b/release/signing/Makefile


Hmm, you're using the same Makefile to do signing on Mac? That seems a little square-peg/roundhole-ish. You don't need most of the tools here to do Mac signing, so I'd suggest setting up a different set of targets or a different Makefile altogether for Mac signing. Unless/until we get to a point where we sign both on the same machine I think it's only asking for trouble to have them run through the same targets.


This seems good overall to me, but feedback- because of the Makefile stuff.

	Syed Albiz [:salbiz]
	Comment 30 • 14 years ago

Comment on attachment 488989 [details] [diff] [review]
refine dmg packing/unpacking

(In reply to comment #29)
> Comment on attachment 488989 [details] [diff] [review]
> refine dmg packing/unpacking
> 
> >diff --git a/buildfarm/utils/installdmg.sh b/buildfarm/utils/installdmg.sh
> 
> > # Now we can copy everything out of the mnt directory into the current directory
> >-rsync -av ./mnt/* .
> >+rsync -av ./mnt/ .
> 
> Any particular reason you're changing this?

This line failed when I tested it with File not Found errors due to the way the shell interpreted the asterisk as a special character (or rather, how it didn't). Removing the asterisk still captures the intended functionality (grab everything inside mnt/, without creating mnt itself (which AFAIK is what the slash does), without the special character to mess things up.

Removing feedback until I get the Makefile issues sorted

	Rail Aliiev [:rail]
	Comment 31 • 14 years ago

be careful:

>-rsync -av ./mnt/* .
this one doesn't copy hidden files

>+rsync -av ./mnt/ .
this one does.

	Syed Albiz [:salbiz]
	Comment 32 • 14 years ago

In addition to the Makefile changes, I've also made a couple of changes to the verification scripts to address a few issues that came up during testing. Right now, the plan to integrate mac signing into regular releases is to have a dedicated mac signing box with the GPG keys and code signing cert in a keychain.

After repacks are done, the windows and mac signing can both be kicked off in parallel, with the windows signing box codesigning the win32 binaries and generating the detached sigs/checksums for both win32/linux/linux64, and mac singing box codesigning the mac/mac64 binaries and generating the detached sigs and checksums for mac/mac64.

In order to integrate this with regular releases, I will also need to add a builder to merge the MD5SUMS/SHA1SUMS files generated by the two signing processes, and modify the FtpPoller to look for both the win32 and mac signing logs before proceeding.

	Syed Albiz [:salbiz]
	Comment 33 • 14 years ago

accidentally included older patch.

	bhearsum@mozilla.com (:bhearsum)
	Comment 34 • 14 years ago

Comment on attachment 489969 [details] [diff] [review]
refresh patch

Makefile stuff:
Can you do autodetection of the OS? Testing for /Users is probably sufficient.
There's a few rules where the only difference is how mail is sent. Can you factor that part out and make the rules work on both OS'? Specifically: create-mac-sigs, verify-mac-sigs, and fake-upload-mac.
We're going to need a different place to upload the existing *SUMS files, too, otherwise we'll have to overwrite them later, which is bad form.
Please rename download-exclude.list to download-win32-exclude.list.

Signing script stuff:
Can you add a comment on why XUL is a special case on Mac?
I think you can make _sign_special_case non-Mac specific, too. If it has to Mac-specific, name the variable to reflect that.

If CodeResources are expected artifacts we should test that they exist rather than removing them. Are they created in any predictable manner?

Nit: Put --no-check-certificate before the file you're downloading


feedback+ in general, but the Makefile still needs some reworking.

	Chris AtLee [:catlee]
	Comment 35 • 14 years ago

Comment on attachment 489969 [details] [diff] [review]
refresh patch

> # Now we can copy everything out of the mnt directory into the current directory
>-rsync -av ./mnt/* .
>+rsync -av ./mnt/ .

Have you checked that this doesn't break any of our existing uses of installdmg.sh?

>+setup-mac:
>+	cvs -d:ext:cltbld@cvs.mozilla.org:/mofo co -d signing release/signing/tools
>+	cvs -d:ext:cltbld@cvs.mozilla.org:/mofo co -d stage release/stage
>+	cvs -d:ext:cltbld@cvs.mozilla.org:/mofo co -d key-checkout release/keys/pgp
>+	wget -O pkg-dmg $(HGROOT)$(REPO)/raw-file/$(TAG)/build/package/mac_osx/pkg-dmg --no-check-certificate
>+	cp ../../buildfarm/utils/installdmg.sh .

I think we're going to have to have the tools directory defined as a variable
that defaults to $(HOME)/tools.

>+++ b/release/signing/download-mac-exclude.list
>@@ -0,0 +1,17 @@
>+- *.zip
>+- *.partial.mar
>+- *.asc
>+- *.tests.tar.bz2
>+- *.exe
>+- *.tar.bz2
>+- /contrib/
>+- /contrib-localized/
>+# Skip the root update directory
>+- /update/
>+# Skip everything that's not a mac unsigned build
>+- /win32/
>+- /mac/
>+- /linux-i686/
>+- /unsigned/win32/
>+- /partner-repacks/
>+- /unsigned/update/win32/

How about '+ /unsigned/mac', and '- *'?

>+def signmacfile(filename, keydir):
>     basename = os.path.basename(filename)
>     dirname = os.path.dirname(filename)
>     stdout = tempfile.TemporaryFile()
>+    command = ['codesign',
>+        '-s', MAC_SIGNING_IDENTITY,
>+        '--keychain', '%s' % keydir,

'%s' % keydir is unnecessary, you can use '--keychain', keydir I think.

>@@ -491,10 +526,13 @@ if __name__ == "__main__":
>     if not os.path.exists('checkouts/stubs'):
>         parser.error("No checkouts/stubs directory found")
> 
>     if options.concurrency == 1:
>         logging.basicConfig(level=logging.INFO, format="%(message)s")
>     else:
>         logging.basicConfig(level=logging.INFO, format="[%(process)d] %(message)s")
> 
>+    MAC_SIGNING_IDENTITY = options.mac_identity
>+    MAC_SIGNING_IDENTIFIER = options.mac_identifier

Having these global variables used in signmacfile makes me nervous.  Can we
pass these to Signer instead, which can pass it down to signmacfile?

	LegNeato
	Comment 36 • 14 years ago

I'm going to put these comments here, feel free to break out into individual bugs:

1) When generated on 10.5 (like http://people.mozilla.com/~salbiz/en-US/), we get the "old" style signature on disk. 10.5-style looks like:

[whatever].app/
  CodeResouces
  Contents/
    ...

While 10.6 looks like:

[whatever].app/
  CodeResouces -> Contents/_CodeSignature/CodeResources
  Contents/
    _CodeSignature/
        CodeResources

Not sure if it makes much of a difference but thought I would mention it.

2) We should have a way to manually specify rules in the CodeResources file. We'll need to use those to omit files from the codesignature checking. I think we need at the very minimum the blocklist xml and anything in updates/. Essentially, anything that may be written into the signed app bundle after signing needs to be ignored, unless it delivers an updated CodeResources file.

As an example, here's a rule from Safari:

                <key>^Resources/.*\.lproj/SafariHelp/</key>
                <dict>
                        <key>omit</key>
                        <true/>
                        <key>weight</key>
                        <real>50</real>
                </dict>

Here they are making the signature verification ignore any help content. They need to do this as help content is updated periodically in the background. They can't deliver a newer CodeResources with the new help content's checksums because users may be running multiple versions of safari with different checksums...there is no way to specify multiple valid checksums for a given path in CodeResources.

	Syed Albiz [:salbiz]
	Comment 37 • 14 years ago

This addresses the issues brought up above, The signing code places the CodeResources artifacts in 10.6 form regardless of where it's run, with the symlink for back compatibility. A custom resource rules file is used to specify what to sign, and so far I've only included the default stuff in Resources and all binaries (dylibs, XUL and firefox-bin) in the seal.

This patch also addresses the tools side of integrating this with release automation, buildbotcustom and eventually configs will also need an update.

	Chris AtLee [:catlee]
	Comment 38 • 14 years ago

Comment on attachment 495161 [details] [diff] [review]
integrate with release automation, use custom resource rules

Do you need to be checking signed_platforms in makeReleaseRepackUrls ?

It would be nice if the platform specific makefile targets had either win32 or mac in them, e.g. 'sign-win32' instead of just 'sign'

	bhearsum@mozilla.com (:bhearsum)
	Comment 39 • 14 years ago

Comment on attachment 495161 [details] [diff] [review]
integrate with release automation, use custom resource rules

I echo both of Catlee's comments, particularly about checking signed_platforms

Can you generalize the rsync stuff in util.commands? You should be able to do do the ssh key handling there, too. Also, use ssh -oIdentityFile= instead of -i -- the latter doesn't expand ~/ properly.

Can you factor out the merge+sort part of merge_files to a different function? Doing so should make it easier to change/extend this to merge large swaths of checksums files down the road (eg, bug 607399).

The docstring of combine_sums.py looks out of date.

Why do we need an exclude and an include rsync file for Mac? Seems like we should just use an exclude one, like Windows. Let me know if you're having trouble getting it to exclude everything you need it to.

You should use rsync --delete when uploading merged checksums -- I'd also recommend --include=*SUMS* --exclude=* to make sure you don't modify anything but sums files.

By and large this looks ok. I'd like to have a run through of it myself after the above is addressed.

	Syed Albiz [:salbiz]
	Comment 40 • 14 years ago

buildbotcustom integration. post_signing FtpPoller needs to look for artifacts for all signed platforms, and builds and repacks have a configurable way of uploading to the unsigned/ sub-dir.

This is dependant on the tools-side integration landing, as that affects the l10n repacks and post_upload.py.

	Syed Albiz [:salbiz]
	Comment 41 • 14 years ago

This separates out the tools side automation integration from changes to the signing scripts. These should be good to land in advance of the changes of the signing scripts, since they default to only treating win32 as a signed platform as usual. Once signing script changes are OK to land and we have a mac signing box set up, the switch can be flipped in the release configs.

	juan becerra [:juanb]
	Comment 42 • 14 years ago

What should be visible signs the binaries are signed? For example, if you compare the copyright information, between Firefox and Chrome, which appears in the application information (cmnd-i) you could argue that the one for Firefox should say something about Mozilla Corp., but it doesn't say anything.

Do you have a list of things I should be checking for with a human eye?

	Syed Albiz [:salbiz]
	Comment 43 • 14 years ago

(In reply to comment #42)
> What should be visible signs the binaries are signed? For example, if you
> compare the copyright information, between Firefox and Chrome, which appears in
> the application information (cmnd-i) you could argue that the one for Firefox
> should say something about Mozilla Corp., but it doesn't say anything.
> 
> Do you have a list of things I should be checking for with a human eye?

As discussed on IRC, the way we've been verifying these internally is running codesign -v <filename> on the app + sealed app internals. Looking at the CodeResources file tells you what files are sealed into the signature. I believe the info string is set in the Info.plist key CFBundleGetInfoString, but this is unrelated to the signing process.

	Chris AtLee [:catlee]
	Comment 44 • 14 years ago

Comment on attachment 496640 [details] [diff] [review]
integrate multiple signed platforms in buildbotcustom

>diff --git a/changes/ftppoller.py b/changes/ftppoller.py
>--- a/changes/ftppoller.py
>+++ b/changes/ftppoller.py
>@@ -113,16 +113,55 @@ class FtpPoller(FtpPollerBase):
>                 self.gotFile = 0
>         # scenario 2:
>         # gotFile is 0, we are waiting for a match to trigger build
>         if self.gotFile == 0:
>             if re.search(self.searchString, pageContents):
>                 self.gotFile = 1
>                 return True
> 
>+class MultiFtpPoller(FtpPollerBase):
>+    """Look for multiple files under the same ftpURLs
>+       Fire a sendchange when all files appear under at least one of the URLs"""

I don't think this actually fires a sendchange.

>+    gotFiles = True

Defaulting to True when you end up setting it to 0/1 below.  Can we use True/False, or True/False/None?

Looks good otherwise.

	bhearsum@mozilla.com (:bhearsum)
	Comment 45 • 14 years ago

Comment on attachment 496645 [details] [diff] [review]
tools side integration

create-release-repacks.py already has access to the release config, so don't add a new flag for this. Check for the platform in releaseConfig['signed_platforms'] instead.

Seems fine other than that, I'd like to see the results of l10n in a staging run once you've got the above fixed.

	bhearsum@mozilla.com (:bhearsum)
	Comment 46 • 14 years ago

Comment on attachment 496640 [details] [diff] [review]
integrate multiple signed platforms in buildbotcustom

What's the point in adding signedPlatformsFiles the release config? Because the signing Makefile doesn't pull the log file names from the release config I think it's better to keep them in release.py. We're hardcoding them either way, and doing it in release.py is more centralized.

l10n stuff needs updating after changing create-release-repacks.py per my previous comment.

Looks fine otherwise.

	Syed Albiz [:salbiz]
	Comment 47 • 14 years ago

Done, will run it through staging to make sure it works as expected.

	Syed Albiz [:salbiz]
	Comment 48 • 14 years ago

Should address all the comments brought up above.
releaseConfig['signedPlatforms'] will need to be set in the releaseConfigs before this can be deployed, unless we're ok with adding some additional logic to create-release-repacks.py to special case signedPlatforms to a default if it isn't set in the configs.

	bhearsum@mozilla.com (:bhearsum)
	Comment 49 • 14 years ago

Comment on attachment 497982 [details] [diff] [review]
fix tools-side automation integration

(In reply to comment #48)
> Created attachment 497984 [details] [diff] [review]
> fix buildbotcustom integration
> 
> Should address all the comments brought up above.
> releaseConfig['signedPlatforms'] will need to be set in the releaseConfigs
> before this can be deployed, unless we're ok with adding some additional logic
> to create-release-repacks.py to special case signedPlatforms to a default if it
> isn't set in the configs.

I think it's OK to assume it's set.

This patch seems fine now!

	bhearsum@mozilla.com (:bhearsum)
	Comment 50 • 14 years ago

Syed, is there anything left to do here besides land it?

	LegNeato
	Comment 51 • 14 years ago

I dropped the ball here. I needed to:

1 - Check with the security team that excluding everything by default and only including the files we know about is ok

2 - Check with rs to get confirmation about what files can change in the bundle / where ISVs/3rd parties can potentially stick extensions, etc.

CCing some folks.

	Robert Strong (they/them - no direct email)
	Comment 52 • 14 years ago

cc'ing Kev in case he has input

We support adding files in the distribution directory though I am not familiar with the structure. I'm not sure if we support adding files in the app's extensions directory for distributions.

	Kev Needham [:kev]
	Comment 53 • 14 years ago

Only question I have around this centers around pkg-dmg, which we use for creating repacked .dmg's from the original installers. I'm going to assume that, since we unpack and repack everything for OSX DMGs already, nothing will change with regards to creating the repack .dmg. We'll have to integrate signing into both the partner repacks and BYOB, and we'll need to acquire a code-signing cert for BYOB as well (which is what we've done for the win32 builds).

Repacks modify the dmg in two places. As rstrong states, we add a distribution directory into Contents/MacOS/, and can add unpacked extensions into Contents/MacOS/extensions as well (but moving fwd we're going to be bundling them in the distribution directory).

Structure of the distribution directory will hopefully not matter, but the nickel tour is:

distribution/distribution.ini  primary config file
distribution/bundle - bundled extensions, only addon installer currently (see bug 392251)
distribution/extensions - bundled extension xpis
distribution/searchplugins - bundled search plugins

Don't think there's huge impact, but would ask that RelEng review current partner-repacks scripts, and ensure they'll take this change up as well.

	LegNeato
	Comment 54 • 14 years ago

Ok, so it sounds like the most secure way is likely to specifically exclude Contents/MacOS/extensions and Contents/MacOS/distribution and require everything else we generate. This is slightly different than the previous plan, which was ignoring * and then requiring a signature for every file we know about. The new plan will make files dropped in the app bundle in places we don't expect to break the seal. The old plan allowed them to exist and not break the seal (signature would still prevent people tampering with our binaries and resources though).

(In reply to comment #53)
> Only question I have around this centers around pkg-dmg, which we use for
> creating repacked .dmg's from the original installers. I'm going to assume
> that, since we unpack and repack everything for OSX DMGs already, nothing will
> change with regards to creating the repack .dmg. We'll have to integrate
> signing into both the partner repacks and BYOB, and we'll need to acquire a
> code-signing cert for BYOB as well (which is what we've done for the win32
> builds).

Would we need a different key? I see two options:
a) Sign Firefox resources with Mozilla's key, but not include anything in extensions or distribution in the signature
b) Resign the entire bundle with a different key

I think arguments can be made for both.

Rob, does Firefox edit the app bundle in any other way (blocklist, updates, unpacking extensions, etc)? Or is that all in the profile?

	Robert Strong (they/them - no direct email)
	Comment 55 • 14 years ago

App update on Mac adds active-update.xml and updates.xml along with an updates directory all in Contents/MacOS. I will likely be changing that after Firefox 4.

The add-ons manager does not install into the Contents/MacOS/extensions/ directory but I suspect there have been third parties that do so as well as corporate distributions.

The blocklist.xml in Contents/MacOS is not updated by the blocklist service... instead, the blocklist.xml in the profile is created / updated.

I wouldn't be surprised if corporate distributions added / deleted the search plugins under searchplugins.

The same goes for dictionaries in the dictionaries directory.

Corporate distributions often lock prefs creating a mozilla.cfg (typically added to Contents/MacOS) and adding a general.config.filename pref (typically added to one of our pref files or to a new pref file) that points to the mozilla.cfg... this is currently broken :( - see bug 595522.

That is a quick brain dump and there may be others.

	Robert Strong (they/them - no direct email)
	Comment 56 • 14 years ago

(In reply to comment #55)
> App update on Mac adds active-update.xml and updates.xml along with an updates
> directory all in Contents/MacOS. I will likely be changing that after Firefox
> 4.
Should have mentioned there are several files that get added to the updates directory while performing an update. If you need more info on the files in the updates directory let me know.

	Syed Albiz [:salbiz]
	Comment 57 • 14 years ago

(In reply to comment #56)
> (In reply to comment #55)
> > App update on Mac adds active-update.xml and updates.xml along with an updates
> > directory all in Contents/MacOS. I will likely be changing that after Firefox
> > 4.
> Should have mentioned there are several files that get added to the updates
> directory while performing an update. If you need more info on the files in the
> updates directory let me know.

I've compiled a list of files (so far) to omit/optionally allow:
Contents/MacOS/active-update.xml
Contents/MacOS/updates.xml
Contents/MacOS/updates/*
Contents/MacOS/extensions/*
Contents/MacOS/distribution/*
Contents/MacOS/mozilla.cfg -> and possibly altering other pref files to add a general.config.filename pref?

If arbritrary pref files may be legitimately modified, perhaps those should all be excluded from the seal as well? Otherwise, is this list of exceptions complete, or could there be more? 

Christian, going forward, do you think our signing scripts should be generating the seal based on your comment #54 (include everything in the package under the seal with just the above exceptions, no new additions without a new seal), or fall back to something more like originally planned (include files explicitly, omit some stuff, allow any new files to be added)?

	LegNeato
	Comment 58 • 14 years ago

(In reply to comment #57)
> Christian, going forward, do you think our signing scripts should be generating
> the seal based on your comment #54 (include everything in the package under the
> seal with just the above exceptions, no new additions without a new seal), or
> fall back to something more like originally planned (include files explicitly,
> omit some stuff, allow any new files to be added)?

I think the original plan is less brittle (and thus better) but I'd like the security team to weigh in as it is arguably more permissive in that case. A fair amount of the security team is out on vacation until the new year though.

	Syed Albiz [:salbiz]
	Comment 59 • 14 years ago

mostly bitrot refresh, with some changes to account for automated signing.

	Syed Albiz [:salbiz]
	Comment 61 • 14 years ago

At this point, the only further changes that should be necessary to the signing scripts are tweaking the generateCodeResourcesFile() function in signing.py or scrapping it entirely for a static CodeResources file, based on what the verdict is. The rest of the functionality has been run through on staging and should not change based on that decision.

	Syed Albiz [:salbiz]
	Comment 62 • 14 years ago

Once the blocking items for this bug are done, this patch turns on mac signing in the release automation. If we want to stagger the landing of the integration, we can also do that by removing macosx64 from the signedPlatforms entry.

	bhearsum@mozilla.com (:bhearsum)
	Comment 63 • 14 years ago

At Catlee's urging, I pinged release-drivers about whether or not we want this for Firefox 4 and the answer was a solid "no". We'll come back to this at some point, but for now, I'm going to removing the feedback/review requests since this isn't a priority.

	Doug Grinbergs
	Comment 64 • 14 years ago

FWIW, 10.7 codesign has slightly different output:

107-11A390-LaCie250:Sandbox doug$ codesign -vv Firefox.app/
Firefox.app/: code object is not signed at all
In architecture: i386

than 10.6:
…Firefox 4.0b12/Firefox.app: code object is not signed

	Chris Cooper [:coop] (he/him)
	Comment 65 • 14 years ago

(In reply to comment #63)
> At Catlee's urging, I pinged release-drivers about whether or not we want this
> for Firefox 4 and the answer was a solid "no". We'll come back to this at some
> point, but for now, I'm going to removing the feedback/review requests since
> this isn't a priority.

bhearsum: could we land everything and pref it off for now?

	bhearsum@mozilla.com (:bhearsum)
	Comment 66 • 14 years ago

These patches aren't ready to land. They need unbitroting and at least another tag -> signing test before we could even land them prefed off. I don't think landing them prefed off would satisfy this bug, anyways.

	John O'Duinn [:joduinn] (please use "needinfo?" flag)
	Comment 67 • 14 years ago

From triage, with coop, catlee, rail, joduinn

This bug is about having release automation capable of signing OSX universal builds in a similar manner to how we sign win32 builds. We will initially support the intel32/64 universal builds from Firefox 4 and greater.

Once this work is completed and verified by RelEng, we will land this in production, pref'd off for now. Tweaking summary to match.

Before RelEng can pref-on this for a live shipping release, there are many product decisions to be worked through. Some include:
* what release do we want to first ship signed OSX builds (be mindful of other features in that release; upcoming osx10.7 release, etc, etc)
* what info support needs in advance?
* QA signoff
** does this work on all versions of OSX?
** can user on unsigned build update to a signed build?
** are there any sideeffects when installing plugins/addons - especially if the addon/plugin adds files into the Firefox directory?
* anything else?

	bhearsum@mozilla.com (:bhearsum)
	Comment 68 • 13 years ago

Erick is going to work on this!

	bhearsum@mozilla.com (:bhearsum)
	Comment 69 • 13 years ago

Comment on attachment 500365 [details] [diff] [review]
refresh tools integration

I think all of this got implemented in a different bug.

	bhearsum@mozilla.com (:bhearsum)
	Comment 70 • 13 years ago

Comment on attachment 500364 [details] [diff] [review]
refresh buildbotcustom integration

This won't be needed anymore.

	bhearsum@mozilla.com (:bhearsum)
	Comment 71 • 13 years ago

Comment on attachment 500406 [details] [diff] [review]
turn on signing in the release configs

Implemented elsewhere.

	bhearsum@mozilla.com (:bhearsum)
	Comment 72 • 13 years ago

Comment on attachment 500403 [details] [diff] [review]
signing script changes

Review of attachment 500403 [details] [diff] [review]:
-----------------------------------------------------------------

::: release/signing/Makefile
@@ -52,1 @@
>  KEYDIR         ?= d:/2010-keys

This file isn't needed anymore.

::: release/signing/combine_sums.py
@@ +1,1 @@
> +#!/usr/bin/env python

This file isn't needed anymore.

::: release/signing/download-mac-exclude.list
@@ +1,1 @@
> +- *.zip

This file isn't needed anymore.

::: release/signing/download-mac-include.list
@@ +1,1 @@
> ++ *.dmg

This file isn't needed anymore.

::: release/signing/download-exclude.list
@@ -1,1 @@
>  - *.zip

This file isn't needed anymore.

::: release/signing/verify-signature.py
@@ -2,1 @@
>  # Verifies that a directory of signed files matches a corresponding directory

This file isn't needed anymore.

	Erick Dransch [:edransch] Assignee
	Comment 73 • 13 years ago

Not final!
TODO:
- Still contains CodeResources code (this should be moved to the build system)
- Doesn't pack/unpack the .tar on the client side yet.

	bhearsum@mozilla.com (:bhearsum)
	Comment 74 • 13 years ago

Comment on attachment 593569 [details] [diff] [review]
Preliminary Patch

Review of attachment 593569 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good overall, you've done a good job merging the original patch into the signing server. There's some comments inline that need to be addressed, and I'll want to play around with the server with these patches applied before giving it an r+, but this is looking really good so far!

::: release/signing/signing.py
@@ +363,4 @@
>      else:
>          raise ValueError("Unknown file type: %s" % filename)
>  
> +def shouldSign(filename, platform='win', product='firefox'):

I'm not a fan of 'win' as a platform here - it's inconsistent with most other things. Use 'win32' instead.

@@ +374,5 @@
>      ext = os.path.splitext(filename)[1]
>      b = os.path.basename(filename)
> +    if platform == 'mac':
> +        product = product + '.app'
> +        if b == product:

I don't think this product check is going to work. For the .app, the capitalized version of the product name is used. For now, just use product.title(). That won't work for SeaMonkey...but we'll cross that bridge later.

@@ +376,5 @@
> +    if platform == 'mac':
> +        product = product + '.app'
> +        if b == product:
> +            return True
> +    else:

Let's be pickier here and look for 'platform in ('win32', 'win64')'. I don't want to use "win" because it's not very consistent with what we do in other places.

@@ +826,5 @@
> +                os.path.join(filename, 'Contents/CodeResources'),
> +                os.path.join(filename, 'Contents/_CodeSignature/CodeResources'))
> +            os.symlink(
> +                os.path.join(filename, 'Contents/_CodeSignature/CodeResources'),
> +                os.path.join(filename, 'Contents/CodeResources'))

Everything in this if statement can go, since the build system will be handling these files.

@@ +852,5 @@
> +    try:
> +        # Unpack it
> +        logs.append("Unpacking %s to %s" % (pkgfile, tmpdir))
> +        unpacktar(pkgfile, tmpdir)
> +        generateCodeResourcesFile(findfiles(tmpdir), code_resources)

This can go too, of course.

@@ +879,5 @@
> +#                os.rename("%s.out" % f, f)
> +#                updated = True
> +#        log.debug("%s Successfully updated manifest", updated)
> +#        log.debug("join %s.app", product)
> +#        tmpdir = os.path.join(tmpdir, "%s.app" % product)

Yeah, anything like this should be handled by the build system....and update.manifest is only present in MAR files anyways...

::: release/signing/signscript.py
@@ +78,5 @@
> +            parser.error("dmg_keydir required when format is dmg")
> +        if not options.mac_id:
> +            parser.error("mac_id required when format is dmg")
> +        if not options.code_resources:
> +            parser.error("code_resources required when format is dmg")

This code_resources stuff can go, right?

Also, can you update signscript.ini.template dmg_keydir and mac_id entries?

	Kohei Yoshino
	Comment 75 • 13 years ago

FYI http://www.apple.com/macosx/mountain-lion/security.html

	bhearsum@mozilla.com (:bhearsum)
	Comment 76 • 13 years ago

Until now this bug has been focused on signing Mac builds in the way that works for apps up through 10.7. I want to continue to keep this the focus. I'll be filing a new bug for 10.8 signing support and we can track any additional work we need to do for that, there. 10.8 doesn't come out until the summer so we have lots of time to deal with it.

	bhearsum@mozilla.com (:bhearsum)
	Comment 77 • 13 years ago

bug 727895 tracks signing-in-a-way-that-supports-10.8.

	Erick Dransch [:edransch] Assignee
	Comment 78 • 13 years ago

The 'codesign' tool for signing mac applications requires the use of an id/key from a keychain. When the 'codesign' command is executed from the command line, a separate user prompt will pop up when codesign tries to access the specified keychain and ask for the keychain's password.

Since we'd like to run without user interaction, we need a way to work around this popup. Any solution will certainly involve a keychain and id dedicated to signing. We've come up with two potential options to provide access to the keychain without user interaction:

1)  Use the 'security unlock-keychain' command right before signing, and use 'security lock-keychain' immediately after signing. The 'security' command accepts the password from standard input, so no user popup will be shown.

2)  Use Applescript to access the user prompt, enter the password, and press enter.
 
In both of these cases, I'm not sure exactly what scope the keychain access will be opened to, access might be limited to the user that executes the command, or to the terminal session. I don't think it's system wide in either case.

	Erick Dransch [:edransch] Assignee
	Comment 79 • 13 years ago

Code that is signed on 10.7 will not validate on 10.5
Code signed on 10.6 verifies across all of 10.[5-7] so we will be signing on 10.6 for the time being.

We will need to re-evaluate once we have a copy of 10.8

	Erick Dransch [:edransch] Assignee
	Comment 80 • 13 years ago

Second iteration. Still waiting for decision on how to handle keychain access (marked with TODO's in code).

We still also need to update signing.ini or signscript.ini with the keychain and mac_id which contain the credentials (once we have them).

Not ready for landing.

	Erick Dransch [:edransch] Assignee
	Comment 81 • 13 years ago

Updated patch to no longer use explicit product name. Instead we will sign any 'app' at the top level of the tar.

	bhearsum@mozilla.com (:bhearsum)
	Comment 82 • 13 years ago

Comment on attachment 599216 [details] [diff] [review]
Updated patch - remove product

Review of attachment 599216 [details] [diff] [review]:
-----------------------------------------------------------------

I'm really happy with this overall. Other than the minor nits below this is ready, modulo-whatever infrasec has to say about unlocking the keychain.

::: release/signing/signing.py
@@ +385,5 @@
> +        if ext in ('.dll', '.exe') and not any(fnmatch.fnmatch(b, p) for p in _dont_sign):
> +            return True
> +    else:
> +        #We should never get here.
> +        log.debug("Invalid Platform")

Please log the platform name here.

@@ +862,5 @@
> +
> +        # Repack it
> +        logs.append("Packing %s" % dstfile)
> +        tar_dir(dstfile, tmpdir)
> +        return 1, 0, 1

What's with the return value? It doesn't look like it's used by the caller.

::: release/signing/signscript.py
@@ +76,5 @@
> +        if not options.dmg_keydir:
> +            parser.error("dmg_keydir required when format is dmg")
> +        if not options.mac_id:
> +            parser.error("mac_id required when format is dmg")
> +        tmpfile = tmpfile 

This seems a little redundant...

::: release/signing/signtool.py
@@ +149,3 @@
>          log.debug("doing %s signing", fmt)
> +        files = []
> +        if fmt == "dmg":

A brief comment explaining why dmg signing is different would be good here.

@@ +182,5 @@
> +                unpacktar(fd+'.tar', os.getcwd())
> +                os.unlink(fd+'.tar')
> +                
> +
> +def packtar_app(tarfile, files):

Since there's only one statement in this method I don't think there's any point in having it. Just make the packtar() call directly instead.

	Erick Dransch [:edransch] Assignee
	Comment 83 • 13 years ago

The 'security unlock keychain' command will unlock the keychain only for the current security context.
This means that any ssh connections will not have access to the keychain when the signing-server has unlocked it.

Implementation: We need to make sure that we don't run into any race conditions where a thread will lock another out of the keychain when it calls 'security lock-keychain'.

	Erick Dransch [:edransch] Assignee
	Comment 84 • 13 years ago

Changed the dmg_signfile function to use pexpect to enter the password into the 'security unlock-keychain' command and avoid using standard in entirely.

Question: should we import pexpect and mercurial only in the 'dmg_signfile' function? We only use them in the one function, and it might save us from installing pexpect/mercurial on our other signing servers.

	bhearsum@mozilla.com (:bhearsum)
	Comment 85 • 13 years ago

Comment on attachment 600487 [details] [diff] [review]
patch to build-tools

Review of attachment 600487 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Erick Dransch [:edransch] from comment #84)
> Question: should we import pexpect and mercurial only in the 'dmg_signfile'
> function? We only use them in the one function, and it might save us from
> installing pexpect/mercurial on our other signing servers.

Yeah, that's a really good idea. r=me if you do that and change the dmg_signfile signature, noted below.

::: release/signing/signing.py
@@ +807,5 @@
>          data = stdout.read()
>          log.exception(data)
>          raise
> +
> +def dmg_signfile(filename, keydir, signing_identity, code_resources, pkgdir, fake=False, passphrase=None):

It looks like pkgdir is only used to create the lock...can you just pass in 'lockfile' instead? Having 'pkgdir' here makes me think it's used for something else.

	Erick Dransch [:edransch] Assignee
	Comment 86 • 13 years ago

Updated patch to address feedback.

	bhearsum@mozilla.com (:bhearsum)
	Comment 87 • 13 years ago

Comment on attachment 600929 [details] [diff] [review]
patch to build-tools

Review of attachment 600929 [details] [diff] [review]:
-----------------------------------------------------------------

::: release/signing/signing.py
@@ +807,5 @@
> +
> +def dmg_signfile(filename, keydir, signing_identity, code_resources, lockfile, fake=False, passphrase=None):
> +    """ Sign a mac .app folder
> +    """
> +    from mercurial import lock, error

Hmm, I hadn't noticed that you were using Mercurial for a lock here. It's sort of a weird dependency for the signing server to have. Is there a different module we can use?

	Erick Dransch [:edransch] Assignee
	Comment 88 • 13 years ago

Use http://packages.python.org/flufl.lock/ instead of mercurial's lock for mutual exclusion around the 'security' and 'codesign' commands.

	bhearsum@mozilla.com (:bhearsum)
	Comment 89 • 13 years ago

Comment on attachment 601391 [details] [diff] [review]
patch to build-tools

Looks good to me!

	Erick Dransch [:edransch] Assignee
	Comment 90 • 13 years ago

Turns out the previous patch won't run, the timeout format passed to lock() needs to be a datetime.timedelta.

	Erick Dransch [:edransch] Assignee
	Comment 91 • 13 years ago

Updated diff against latest version of build-tools.

	bhearsum@mozilla.com (:bhearsum)
	Comment 92 • 13 years ago

Comment on attachment 601596 [details] [diff] [review]
final patch - build-tools

Review of attachment 601596 [details] [diff] [review]:
-----------------------------------------------------------------

Landed this. I'm updating the dep signing servers first. Once I've verified that everything is still OK after the upgrade I'll do the rest.

	bhearsum@mozilla.com (:bhearsum)
	Comment 93 • 13 years ago

OK, all instances have been updated and restarted. I also updated the SIGNING_SERVER tag to point at the updated code. Only thing left to do in this bug is enable it on the Buildbot side, which is dependent on bug 723176 and requires an additional patch.

	bhearsum@mozilla.com (:bhearsum)
	Comment 94 • 13 years ago

Had to back this out because of issues on the clients:
Traceback (most recent call last):
  File "e:/builds/moz2_slave/m-cen-w32/tools/release/signing/signtool.py", line 12, in <module>
    from signing import remote_signfile, find_files, buildValidatingOpener
  File "e:\builds\moz2_slave\m-cen-w32\tools\release\signing\signing.py", line 853
    except TimeOutError as error:
                         ^


I think Python 2.5 doesn't support that syntax. I left the servers alone to avoid the churn.

	Erick Dransch [:edransch] Assignee
	Comment 95 • 13 years ago

Change exception handling to correct python2.5 syntax. This syntax remains valid in 2.6 and 2.7.

	Erick Dransch [:edransch] Assignee
	Comment 96 • 13 years ago

Add mac signing servers to buildbot-configs

	bhearsum@mozilla.com (:bhearsum)
	Comment 97 • 13 years ago

Comment on attachment 603042 [details] [diff] [review]
patch to buildbot-configs

Review of attachment 603042 [details] [diff] [review]:
-----------------------------------------------------------------

I applied this and looked at the output of 'python config.py', and this looks good! Note to self: we need to update the passwords.py that we sync out to the masters before this lands.

	Erick Dransch [:edransch] Assignee
	Comment 98 • 13 years ago

Added sample signscript.ini to demonstrate the settings needed for the new Mac signing changes.

	Erick Dransch [:edransch] Assignee
	Comment 99 • 13 years ago

As per IRC:
Change dmg_keydir to dmg_keychain to better reflect reality.

	Erick Dransch [:edransch] Assignee
	Comment 100 • 13 years ago

The server validates signing of a test file on startup. This file can be used for that validation step.

	bhearsum@mozilla.com (:bhearsum)
	Comment 101 • 13 years ago

Comment on attachment 607221 [details] [diff] [review]
patch to build-tools

This looks fine, and it's been running fine on the new mac signing servers.

	bhearsum@mozilla.com (:bhearsum)
	Comment 102 • 13 years ago

Mass move of bugs to Release Automation component.

	bhearsum@mozilla.com (:bhearsum)
	Comment 103 • 13 years ago

Comment on attachment 603042 [details] [diff] [review]
patch to buildbot-configs

I landed a variant of this, unbitrotted and I removed the mac-nightly-signing servers for the ux branch, because we don't do the same for Windows. (This means that we'll use the dep signing ones instead.)

This has been enabled in production, too.

	bhearsum@mozilla.com (:bhearsum)
	Comment 104 • 13 years ago

Comment on attachment 603042 [details] [diff] [review]
patch to buildbot-configs

Had to back this out because apparently we do MAR signing on Mac, and the signing servers aren't set-up for it.

	bhearsum@mozilla.com (:bhearsum)
	Comment 105 • 13 years ago

Comment on attachment 607221 [details] [diff] [review]
patch to build-tools

This landed ages ago.

	bhearsum@mozilla.com (:bhearsum)
	Comment 106 • 13 years ago

Comment on attachment 603042 [details] [diff] [review]
patch to buildbot-configs

Re-landed this now that the mac signing servers support MAR signing. Will be heading to production shortly.

	bhearsum@mozilla.com (:bhearsum)
	Comment 107 • 13 years ago

OK, so there's nothing left to be done here:
- All the patches have landed
- The production signing servers have .app signing enabled.

bug 723176 has the patch that enables it, which needs to land on a per-branch basis. bug 752613 also has a bunch of notes related to signing with Developer IDs.

	Andy Cunningham
	Comment 108 • 10 years ago

We have discovered that on OS X, the following app within the Firefox bundle also needs to be signed:

/Applications/Firefox.app/Contents/MacOS/plugin-container.app

Otherwise, the end user is prompted every time that certain plugins which require firewall access are initiated. Please see the following bug for details:

https://bugzilla.mozilla.org/show_bug.cgi?id=1050567

	bhearsum@mozilla.com (:bhearsum)
	Comment 109 • 10 years ago

(In reply to Andy Cunningham from comment #108)
> We have discovered that on OS X, the following app within the Firefox bundle
> also needs to be signed:
> 
> /Applications/Firefox.app/Contents/MacOS/plugin-container.app
> 
> Otherwise, the end user is prompted every time that certain plugins which
> require firewall access are initiated. Please see the following bug for
> details:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1050567

This will be dealt with as part of bug 1046306.