The fix for bug 394984 requires that we start signing the actual firefox binary in the app bundle as well as the new org.mozilla.updater. org.mozilla.updater used to simply be called updater and was renamed as part of the patches in bug 394984. It is my belief that the firefox binary cannot be signed in its current form, unless patch part 1 from bug 394984 is applied. Otherwise, signing will fail when trying to sign MainMenu.nib. Attached is part 3 of the patches in bug 394984 that demonstrates how signing worked for me locally while working on bug 394984. This is meant as fyi only. It does not make any claim about how this should be integrated into our build system. These seem to be the necessary steps: 1. Add Firefox requirements to updater's Info.plist 2. Add org.mozilla.updater requirements to Firefox's Info.plist 3. Sign org.mozilla.updater 4. Sign Firefox

We figured out a plan for this, and the updater signing stuff turns out to be all build system work, so I'm moving this over. From https://bugzilla.mozilla.org/show_bug.cgi?id=394984#c62: > > 3. Sign the org.mozilla.updater binary under Contents/Library/LaunchServices > > in the Firefox .app bundle with the certificate that matches the signing > > requirements from point 1 above. > > I think an easier alternative is to sign > Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater, and then copy > it over to Contents/Library/LaunchServices afterwards. I'm actually really > confused about how just signing the LaunchServices one works...I don't see > how it could be taking the updater's Info.plist into account given that. In > any case, I'm certain that we want the two updater binaries to bit for bit > identical. > > Mechanically, this should be pretty easy to do. You'll need to get > $(MOZ_SIGN_CMD) called (when defined, and only on Mac) at some point during > the updater's build process. It looks like > https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/ > updater/Makefile.in#53 may be a good place, but you'll probably know better > than me. Calling it should end up being almost the same as > https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/ > windows/nsis/makensis.mk#49, but you'll want MOZ_INTERNAL_SIGNING_FORMAT > instead of EXTERNAL. > I filed bug 987712 for the buildbot change we'll need to make this possible.

All right, I'm back on this. Ben, are you able to provide the different Subject.OUs? I have '43AQ936H96', is this for nightly? I'm referring to this part of your comment from bug 394984 comment 62: (In reply to Ben Hearsum [:bhearsum] from comment #62) > The subject.OU value differs depending on which certificate the build is > signed with. On-change and Try builds use one value, Nightly/Aurora > nightlies use a different value, and Betas and Releases use another > different value. It's probably O.K. to hardcode this into the tree, and > choose the right value similar to how we choose MAR fingerprints Also, if you could confirm that these three categories are correct (i.e. they cover the three certificates and Subject.OUs), I'd appreciate it. Should I be checking for 'on-change' and 'try' specifically? if CONFIG['MOZ_UPDATE_CHANNEL'] in ('beta', 'release', 'esr'): DEFINES['SIGNING_SUBJECT_OU'] = 'FirstSubjectOU' elif CONFIG['MOZ_UPDATE_CHANNEL'] in ('nightly', 'aurora', 'nightly-elm', 'nightly-profiling', 'nightly-oak'): DEFINES['SIGNING_SUBJECT_OU'] = 'SecondSubjectOU' else DEFINES['SIGNING_SUBJECT_OU'] = 'ThirdSubjectOU'

(In reply to Stephen Pohl [:spohl] from comment #2) > All right, I'm back on this. > > Ben, are you able to provide the different Subject.OUs? I have '43AQ936H96', > is this for nightly? I'm referring to this part of your comment from bug > 394984 comment 62: > > (In reply to Ben Hearsum [:bhearsum] from comment #62) > > The subject.OU value differs depending on which certificate the build is > > signed with. On-change and Try builds use one value, Nightly/Aurora > > nightlies use a different value, and Betas and Releases use another > > different value. It's probably O.K. to hardcode this into the tree, and > > choose the right value similar to how we choose MAR fingerprints > > Also, if you could confirm that these three categories are correct (i.e. > they cover the three certificates and Subject.OUs), I'd appreciate it. > Should I be checking for 'on-change' and 'try' specifically? > > if CONFIG['MOZ_UPDATE_CHANNEL'] in ('beta', 'release', 'esr'): > DEFINES['SIGNING_SUBJECT_OU'] = 'FirstSubjectOU' > elif CONFIG['MOZ_UPDATE_CHANNEL'] in ('nightly', 'aurora', 'nightly-elm', > 'nightly-profiling', 'nightly-oak'): > DEFINES['SIGNING_SUBJECT_OU'] = 'SecondSubjectOU' > else > DEFINES['SIGNING_SUBJECT_OU'] = 'ThirdSubjectOU' Yep! And I just noticed that it's the same for beta/release/nightly: 43AQ936H96 (https://github.com/mozilla/build-puppet/blob/master/modules/toplevel/manifests/server/signing.pp#L66) However, the other ones are still the self signed cert, and the subject OU for those is "Release Engineering".

(In reply to Ben Hearsum [:bhearsum] from comment #3) > (In reply to Stephen Pohl [:spohl] from comment #2) > > All right, I'm back on this. > > > > Ben, are you able to provide the different Subject.OUs? > > Yep! And I just noticed that it's the same for beta/release/nightly: > 43AQ936H96 > (https://github.com/mozilla/build-puppet/blob/master/modules/toplevel/ > manifests/server/signing.pp#L66) > > However, the other ones are still the self signed cert, and the subject OU > for those is "Release Engineering". It turns out that we can just hardcode the subject OU as 43AQ936H96, because the native SMJobBless API will reject any self-signed certs. So even if we were to dynamically adjust the subject OU, elevated updates would fail.

This should capture everything that we've discussed, but I don't think I have the ability to build this on a machine that has access to our "43AQ936H96" signing cert to see if everything works as expected. Ben, would you be able to apply the patch in bug 394984 and this patch here and build it on a system with said cert? Feel free to ping me on IRC with any questions. Thanks!

Comment on attachment 8442151 [details] [diff] [review] Patch (untested) Looks like Oak is using real certs, so I'll try and push this patch there. Thanks for the heads up, Ben!

Is the value '43AQ936H96' something that persists when you need to renew a developer id cert ? That's not due until 2017. AFAICT this would only be something to document, as the Info.plist and updater binary should always be in sync (so no issue with serving updates to old versions).

The 'OU' in Subject.OU stands for Organizational Unit. I would expect this to persist when we renew our certs, since the new cert would be issued to the same organizational unit. However, the rest of the signing requirements could probably change. Here is the example from the updater's Info.plist again as fyi (taken from bug 394984 comment 62): identifier "org.mozilla.nightly" and ( (anchor apple generic and certificate leaf[field.1.2.840.113635.100.6.1.9] ) or (anchor apple generic and certificate 1[field.1.2.840.113635.100.6.2.6] and certificate leaf[field.1.2.840.113635.100.6.1.13] and certificate leaf[subject.OU] = "43AQ936H96")) Where would be the best place to document this?

I've updated RelEng's doc on mana. If we ever implemented bug 366846 we'd be in trouble I think, or would need to also update the plist before trying to run the updater.

This is now *supposed* to work, but it doesn't. The build fails during packaging, when it attempts to sign the updater.app bundle 300 times and ultimately times out. The important step is in toolkit/mozapps/installer/packager.mk, but the build fails like so: https://tbpl.mozilla.org/php/getParsedLog.php?id=42567081&tree=Oak&full=1 I set up a system exactly like our signing servers (10.6.8, XCode 4.2), and I can successfully sign my .app bundle locally. Nick, is there any way to get a log from our signing servers to see why it might be failing there? Another way to debug this is if I could pass a '-v' to signscript.py to get better logging, but the only thing I seem to be able to do is add '-v' to my invocation of signtool.py, which doesn't get verbose feedback from signscript.py.

We debugged a bit on IRC, and I suggested changing directory to $(MOZ_MACBUNDLE_NAME)/Contents/MacOS/ before calling $(MOZ_SIGN_PREPARED_PACKAGE_CMD) updater.app The signing code is only looking for directory entries ending in .app in the top-level of the archive uploaded: http://mxr.mozilla.org/build/source/tools/lib/python/signing/utils.py#320

This patch implements what we've discussed, with one difference that I shall explain in detail: My original thinking was that we could sign updater.app, extract the signed org.mozilla.updater binary and then copy it to Firefox.app/Contents/Library/LaunchServices. The problem here is that extracting the binary from updater.app breaks the signature on org.mozilla.updater. This means that we have to sign org.mozilla.updater by itself. I tried to do this with this patch here, but it appears that we're only supporting signing of .app bundles, not individual files. We might only have to drop the '--resource-rules' argument to codesign in signscript.py for this to work. Interestingly, a run on Oak said that the signing of org.mozilla.updater was successful. But when I run codesign -vvv on the org.mozilla.updater binary in the final FirefoxNightly.app bundle, it tells me that the binary is not signed at all. The full log is here (search for 'uploading' to jump to the place of interest): https://tbpl.mozilla.org/php/getParsedLog.php?id=42945198&tree=Oak&full=1 Nick, do you know of a way to sign individual files on OSX? Or does this require changes to the signing scripts? I'm assuming that the supposedly successful signing of org.mozilla.updater is a false positive, is that right?

Ah, I guess http://mxr.mozilla.org/build/source/tools/lib/python/signing/utils.py#414 has the answer as to why we're not signing the individual binary (it doesn't have a .app file extension). It's a bit confusing that signtool.py says that the signing step was successful when we didn't sign anything at all, but I might be the first one to want to do this.

FWIW if I do codesign -vvv on a binary inside a nightly .app it also says not signed, bug codesign -vvv FirefoxNightly.app it does notice if files are renamed. So another way to put your request is to enable signing of individual files (in a similar way to what happens on windows). We've reach the end of my knowledge on this stuff, so I'm going to pass this question over to bhearsum.

(In reply to Stephen Pohl [:spohl] from comment #12) > Created attachment 8449881 [details] [diff] [review] > Patch (wip) > > This patch implements what we've discussed, with one difference that I shall > explain in detail: My original thinking was that we could sign updater.app, > extract the signed org.mozilla.updater binary and then copy it to > Firefox.app/Contents/Library/LaunchServices. The problem here is that > extracting the binary from updater.app breaks the signature on > org.mozilla.updater. This means that we have to sign org.mozilla.updater by > itself. Can you expand on what you mean by "extract the signed org.mozilla.updater" binary? My eyes read that as "copy that file elsewhere", and I don't understand how that could break the signature.

(In reply to Ben Hearsum [:bhearsum] from comment #15) > Can you expand on what you mean by "extract the signed org.mozilla.updater" > binary? My eyes read that as "copy that file elsewhere", and I don't > understand how that could break the signature. Ben and I have been discussing this on IRC and I ran some tests on Oak. I'll file a separate bug for the necessary server side changes and will take discussion there. Brief summary: we need to add the ability to sign org.mozilla.updater by itself, without any dependencies on the enclosing .app bundle, which requires changes to our signing scripts.

This is technically ready for review, but depends on bug 394984 and the signing server changes before it can be checked in.

Comment on attachment 8453062 [details] [diff] [review] Patch This looks fine to me. My understanding of this stuff is rather limited, though.

Comment on attachment 8453062 [details] [diff] [review] Patch The packager.mk changes won't be needed here since we'll be using the new signing server code, which will dive deep into the package and this binary already. You may still need the Info.plist changes - I don't really know much about that...

Since bug 394984 is unlikely to land in the next two weeks I'm unassigning myself for now.

This was resolved by bug 1046306 and all its dependencies.