1046747 - make sure signing server works on 10.9 and with v2 signatures

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

According to the details in bug 1046306, we need to start signing our OS X binaries on 10.9. This bug is to track any changes to the signing server code needed to make it work on 10.9.

Comment 1

[[e-mail] (:bhearsum)]

Note: 10.9 implies supporting v2 signatures, which probably requires changes to our codesign invocations.

Comment 2

[[e-mail] (:bhearsum)]

From the attachment in bug 1046306:
"Code signing uses extended attributes to store signatures in non-Mach-O executables such as script files. If the extended attributes are lost then the program's identity will be broken. Thus, when you ship your script, you must use a mechanism that preserves extended attributes.

One way to guarantee preservation of extended attributes is by packing up your signed code in a read-write disk image (DMG) file before signing and then, after signing, converting to read-only. You probably don't need to use a disk image until the final package stage so another less heavy-handed method would be to use ZIP or XIP files."

This means that the signing server _must_ return the signed package in a dmg. Something on the slave (signtool.py or the build system) will need to mount the dmg and pull the .app out of it. We could probably still submit files for signing as a .tar.gz, which may save some hassle at the expense of confusion.

I'm concerned about what's going to happen if we have too many different threads on the signing server all creating dmgs at the same time - we used to have issues with multiple mounted volumes at the same time way back in the 10.4 or 10.5 days. It may not be an issue now, but it's something to watch out for.

Comment 3

[[e-mail] (:bhearsum)]

We'll also need to get rid of our usage of "--resource-rules CodeResources". This is how we were able to exclude files/directories such as channel-prefs.js and distribution from the signature. It's important for us to still be able to do that, so we need to investigate alternatives to this.

Comment 4

[[e-mail] (:bhearsum)]

re: extended attributes

We discovered that at least part of the extended attributes are actually not file metadata, but rather stored in separate files that are alongside the .app. Eg:
__MACOSX/Firefox.app/Contents/MacOS/browser/components/._components.manifest

I'm going to try to adjust our signing tools to include these along with the .app. Maybe if we do that, we'll magically retain xattrs? This would save us the effort of using dmg us a transport format.

Comment 5

[[e-mail] (:bhearsum)]

It looks like the extended attributes aren't going to be a problem. OS X's tar will preserve them, which is our transport format. I think Stephen found a way to work around them...but I can't recall.

The trickiest part here is that we need to sign every individual file in Contents/MacOS (and hopefully in Resources and maybe other dirs). However, we also need to handle subapps, which need to be signed on their own. I think I've figured out a way to do this, but I'm still finalizing the patch. I hope to have something up for feedback by EOD.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: support v2 signatures

I want to do some more testing on this (particularly, making sure all the stuff we currently sign will sign properly with it), but this works with spohl's manually constructed Firefox with the new required layout.

The patch looks scary than it is. Most of the changes to dmg_signpackage are just moving the lockfile logic there. git diff -w makes that more clear.

I _think_ the comments in dmg_signfile should be enough to explain what's going on, but let me know if it's not clear enough.

There's an instance running on r5-mini-002 if you want to poke at it. It accepts tokens and signing requests from 127.0.0.1. You should be able to get in as root.

Comment 7

[[e-mail] (:bhearsum)]

Attachment: alter the list of things we sign

I spoke with Stephen again about how we should sign. He says the following should work:
* Sign all inner apps first (same rules as below)
* Sign all individual files in .app/Contents/MacOS (other than the main binary), .app/Contents/Library
* Do NOT sign files any other files (eg, .app/Contents/Resources, .app/MozResources)

The files in .app/Contents/Resources do not get signed individually because they're included in the CodeResources file that gets created by codesign. .app/MozResources is not signed at all, and it contains things such as prefs/ini files that need to be able to change, as well as partner build bits.


I still need to do more tests of this, but I'm asking for review now to minimize delay if everything works out.

Comment 8

[Chris AtLee [:catlee]]

Comment on attachment 8467798 [details] [diff] [review]
alter the list of things we sign

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

just a few nits overall.

do we need to preserve the old signing format?

::: lib/python/signing/utils.py
@@ +19,1 @@
>  """

the spacing here weirds me out

@@ +255,5 @@
> +            abs_f = os.path.join(top, f)
> +            # If we find any ".app" after the app we're currently signing
> +            # this means it's an inner app, which was handled above.
> +            # We shouldn't do any further processing here.
> +            if top.replace(filename, "").count(".app") > 0:

this seems a little bit fragile. wouldn't top[len(filename):] work?

@@ +267,5 @@
> +            # Lastly, we only want to sign individual files in certain
> +            # directories. See SIGN_DIRS above for further explanation.
> +            if top.replace("%s/" % filename, "") not in SIGN_DIRS:
> +                log.debug("Skipping %s because it's not in SIGN_DIRS.", abs_f)
> +                continue

can you delete these directories from dirs to avoid recursing into them?

Comment 9

[[e-mail] (:bhearsum)]

Okay, so we can't sign the current Firefox packages on 10.9 because of errors like:
FirefoxNightly.app: code object is not signed at all
In subcomponent: /Users/cltsign/unsigned/FirefoxNightly.app/Contents/MacOS/application.ini

This means that we'll need to deal with running 10.6 and 10.9 signing servers at the same time. This is going to cause a lot of complications. For example, it means that once we enable the new signing servers for try, mac builds will stop working completely if you're pushing any change that doesn't have the new package layout. It will also be very difficult to land this cleanly. The best bet is likely going to be:
* Close the trees
* Enable new signing servers
* Land package layout change (and hope it lands cleanly)
* Re-open.

We'll have to do this for aurora/beta/release/esr31 when we backport, too.

Comment 10

[[e-mail] (:bhearsum)]

Note: once this patch lands, we should avoid updating the code on the 10.6 signing servers - it won't work there. Should probably create a branch in build/tools and move the checkouts to it.

Comment 11

[[e-mail] (:bhearsum)]

Attachment: mac-sign-tools-3.diff

I addressed your review comments. Trimming the dirs was a bit trickier than expected because I also had to explicitly stop processing files in root. I think this is cleaner despite being a tad lengthier, though.

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 8469250 [details] [diff] [review]
mac-sign-tools-3.diff

Chris is out this week. Rail said he can review this after he's up to speed.

Comment 13

[Rail Aliiev [:rail]]

Comment on attachment 8469250 [details] [diff] [review]
mac-sign-tools-3.diff

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

some nits below:

::: lib/python/signing/utils.py
@@ +246,5 @@
> +    # We scan "Contents" here to make it easier to find inner apps
> +    # (by looking for things that end with ".app")
> +    contents_d = os.path.join(filename, "Contents")
> +    for top, dirs, files in os.walk(contents_d):
> +        for d in dirs:

Can you iterate over a copy of dirs because you remove items from it below.

@@ +252,5 @@
> +            # We only want to sign individual files in certain directories.
> +            # See SIGN_DIRS above for further explanation.
> +            if top == contents_d and d not in SIGN_DIRS:
> +                log.debug("Skipping %s because it's not in SIGN_DIRS.", abs_d)
> +                dirs.remove(d)

Actually you can skip dirs.remove(d) because dirs is not used anywhere.

@@ +358,5 @@
> +                    dmg_signfile(macdir, keychain, mac_id, subject_ou, fake)
> +
> +                except TimeOutError, error:
> +                    # timed out acquiring lock, give an error
> +                    log.exception("Timeout acquiring lock  %s for codesign, is something broken? ", lockfile, error)

Looks like error is not interpolated... Not sure if you need it since log.exception dumps the traceback.

Comment 14

[Rail Aliiev [:rail]]

Comment on attachment 8469250 [details] [diff] [review]
mac-sign-tools-3.diff

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

::: lib/python/signing/utils.py
@@ +252,5 @@
> +            # We only want to sign individual files in certain directories.
> +            # See SIGN_DIRS above for further explanation.
> +            if top == contents_d and d not in SIGN_DIRS:
> +                log.debug("Skipping %s because it's not in SIGN_DIRS.", abs_d)
> +                dirs.remove(d)

It turns out that dirs.remove(d) has a side effect which prevents os.walk() to enter into the removed directories!!!
Can you add some comment here to avoid accidental "improvements". :)

Comment 15

[[e-mail] (:bhearsum)]

Attachment: patch as landed

Per IRC, this is the patch as landed - I've added a comment around the confusing os.walk() behaviour, and removed the unnecessary variable in the exception handling code.

This doesn't affect any currently running signing servers (we absolutely should NOT move the existing mac ones to this code, and it's a no-op for Linux). I've landed this and moved the SIGNING_SERVER tag to it. The new mac signing servers will be brought up with this code.

Comment 16

[[e-mail] (:bhearsum)]

This bug was only about signing server code, and thus is done. bug 1046749 tracks bringing up new 10.9 signing servers running this code, and bug 1049595 tracks the necessary buildbot config changes.