741751 - Installer for Partner Repack for Firefox 11 are not signed

Status: VERIFIED FIXED

(Release Engineering :: Release Requests, defect, P1)

Description

[Carsten Book [:Tomcat]]

seems the installer on http://stage.mozilla.org/pub/mozilla.org/firefox/candidates/11.0-candidates/build2/partner-repacks/ are not signed, while the Firefox 10 Releases were :-/

The files in the installer like firefox.exe and updater.exe are signed, however the installer is not.

Comment 1

[[e-mail] (:bhearsum)]

build1 isn't signed either, which makes this a problem with the release automation, I believe. They installers *do* have GPG sigs though, so it sounds like they're getting to the signing server for *something*.

Comment 2

[Rail Aliiev [:rail]]

So, it looks like signtool.py (actually pefile module) thinks that the file is signed here: http://hg.mozilla.org/build/tools/file/f881687f0ed5/release/signing/signtool.py#l26

We need to remove the invalid signature from the PE header somehow. Investigating...

Comment 3

[Rail Aliiev [:rail]]

Attachment: repack win32 for reals

Comments incoming

Comment 4

[Rail Aliiev [:rail]]

Comment on attachment 612493 [details] [diff] [review]
repack win32 for reals

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

The signatures were broken because we add files to existing installers using "7za a ..." what changes content of the installers but doesn't strip the signature properly.
Python pefile library detects the old signature (but doesn't check it) and prevents signtool.py from uploading the files for signcode signing.

I tried to use the same library to strip the signature, but it didn't work properly. Then I decided to reuse the existing repackSignedBuilds function, but it didn't work out of the box, so I had to add extra logging to debug it. Then I decided to cleanup the script (pep8fy and pylintify a bit). This is why the patch is big.

I've been testing this in staging. So far it looks good: generates the same amount of files with the same names. Still need to verify signatures and installers. Probably will need some help with QA/verification from Tomcat.

See my comments inline.

::: scripts/partner-repacks.py
@@ +12,5 @@
> +
> +logging.basicConfig(stream=sys.stdout, level=logging.INFO,
> +                    format="%(asctime)-15s - %(message)s")
> +log = logging.getLogger(__name__)
> +

I replaced print statements with logging to unify output and have timing.

@@ +44,5 @@
> +    FancyURLopener"""
> +
> +    def http_error_default(self, url, fp, errcode, errmsg, headers):
> +        urllib.URLopener.http_error_default(self, url, fp, errcode, errmsg,
> +                                            headers)

I added this class to get rid of isFileValid function which was returning False for app.tag, which less thank 1000 bytes. It raises exceptions for 4xx, 5xx, but at the same time can handle 3xx.

@@ +52,5 @@
> +    version = re.sub(r'a([0-9]+)$', r' Alpha \1', version)
> +    version = re.sub(r'b([0-9]+)$', r' Beta \1', version)
> +    version = re.sub(r'rc([0-9]+)$', r' RC \1', version)
> +    return version
> +

Copy paste from tools/lib/python. Regexp used here might have "y2k" problems (long versions).

@@ +64,2 @@
>  
> +    fpath = path.dirname(program)

s/os.path/path/ since we have that imported. Drop unused fname.

@@ +74,5 @@
>  
>      return None
>  
> +
> +def rmdirRecursive(directory):

s/dir/directory/ to avoid overriding builtin dir

@@ +123,5 @@
>      # Shell command output gets dumped immediately to stdout, whereas
>      # print statements get buffered unless we flush them explicitly.
>      sys.stdout.flush()
>      p = Popen(cmd, shell=True)
> +    (_, ret) = os.waitpid(p.pid, 0)

Unused rpid, make pylint happy.

@@ +143,3 @@
>  def isLinux(platform):
> +    return 'linux' in platform
> +

More readable, imho. The same applies for other is* functions.

@@ +185,2 @@
>  def createTagFromVersion(version):
> +    return 'FIREFOX_' + str(version).replace('.', '_') + '_RELEASE'

Just a space after coma.

@@ +187,3 @@
>  
> +
> +def parseRepackConfig(filename, platforms):

s/file/filename/ to avoid overriding builtin file()

@@ +193,3 @@
>      for line in f:
>          line = line.rstrip("\n")
> +        [key, value] = line.split('=', 2)

Space after coma

@@ +214,5 @@
> +            ftp_platform = getFtpPlatform(key)
> +            if ftp_platform in [getFtpPlatform(p)
> +                                      for p in platforms] \
> +               and value.lower() == 'true':
> +                config['platforms'].append(ftp_platform)

Reformat to be shorter than 80 chars.

@@ +220,5 @@
>      if config['platforms']:
>          return config
>  
> +
> +def getFtpPlatform(platform):

IMHO, more meaningful name.

@@ +242,5 @@
>          return "win32"
>      return None
>  
> +
> +def getFilename(version, platform, file_ext):

Drop unused locale variable.

@@ +253,3 @@
>  
> +    if isLinux(platform):
> +        return "firefox-%s.%s" % (version, file_ext)

Drop version check for 3.0 (we don't even support 3.6 ;) ), simplify pretty version conversion.

@@ +266,5 @@
>          return "tar.bz2"
>      if isMac(platform):
>          return "dmg"
>      if isWin(platform):
> +        return "exe"

Drop version check.

@@ +274,3 @@
>  class RepackBase(object):
>      def __init__(self, build, partner_dir, build_dir, working_dir, final_dir,
> +                 ftp_platform, repack_info, signing_command, file_mode=0644,

Rewrap, s/platform_formatted/ftp_platform/

@@ +419,5 @@
>          else:
>              quiet_flag = ""
> +        pkg_cmd = "%s --source stage/ --target \"%s\" --volname 'Firefox' " \
> +        "--icon stage/.VolumeIcon.icns --symlink '/Applications':' ' %s" \
> +        % (options.pkg_dmg, self.build, quiet_flag)

Rewrap to fit 80 chars.

@@ +455,4 @@
>  def repackSignedBuilds(repack_dir):
> +    log.info('Repacking signed builds in %s' % repack_dir)
> +    cwd = os.getcwd()
> +    os.chdir(script_directory)

I removed "if" check, I think we should fail if the directory doesn't exist.
chdir to the target directory to prevent repacking the world (find . -name *.exe in the script)

@@ +492,5 @@
> +    log.info('To: %s', file_path)
> +    log.info('CWD: %s' % os.getcwd())
> +    try:
> +        # use URLopener, which handles errors properly
> +        StrictFancyURLopener().retrieve(url, file_path)

Use StrictFancyURLopener which raises on HTTP errors

@@ +515,4 @@
>  if __name__ == '__main__':
>      error = False
>      partner_builds = {}
> +    default_platforms = ['linux-i686', 'linux-x86_64', 'mac', 'win32']

Drop mac64, we don't have it.

@@ +594,5 @@
> +    parser.add_option(
> +        "-q", "--quiet",  action="store_true", dest="quiet",
> +        default=False,
> +        help="Suppress standard output from the packaging tools"
> +    )

Reformat parser.add_option to fit 80 chars (mostly for help="")
Drop action="store" which used by default.

@@ +600,5 @@
>  
> +    if not options.quiet:
> +        log.setLevel(logging.DEBUG)
> +    else:
> +        log.setLevel(logging.WARNING)

Obey --quiet

@@ +647,5 @@
>  
>      if error:
>          sys.exit(1)
>  
> +    base_workdir = os.getcwd()

Drop version check

@@ +657,4 @@
>          win_candidates_web_dir = candidates_web_dir
>      else:
> +        candidates_web_dir = "/pub/mozilla.org/%s/%s-candidates/build%s" % \
> +            (options.nightly_dir, options.version, options.build_number)

Reformat to fit 80 chars.

@@ +663,5 @@
>          else:
>              win_candidates_web_dir = candidates_web_dir + '/unsigned'
>  
>      # Local directories for builds
> +    script_directory = os.getcwd()

repackSignedBuilds needs to know this to download stubs here just one time

@@ +722,4 @@
>                                         file_ext)
>  
> +                local_filepath = path.join(original_builds_dir, ftp_platform,
> +                                           locale)

Get rid of getLocalFilePath which used just once here. version variable wasn't used.

@@ +749,5 @@
>                              candidates_dir = candidates_web_dir
> +                        original_build_url = "http://%s%s/%s/%s/%s" % \
> +                            (options.staging_server, candidates_dir,
> +                             ftp_platform, locale, filename)
> +

Drop version check

@@ +754,5 @@
>                          retrieveFile(original_build_url, filename)
> +                        if isWin(platform) and options.use_signed:
> +                            # The following removes signatures by 
> +                            # repacking the source file
> +                            repackSignedBuilds(os.getcwd())

The main magic is here. We repack *original* installers just once after we retrieveFile them. Only for windows when --signed is used.

@@ -866,5 @@
> -               and not signing_command:
> -                repackSignedBuilds(repacked_builds_dir)
> -            # Remove our working dir so things are all cleaned up and ready for
> -            # easy upload.
> -            printSeparator()

Don't need this anymore, we do this per downloaded original file.

::: scripts/repack-signed.sh
@@ +1,3 @@
>  #!/bin/sh
>  set -e
> +set -x

Just to be verbose

Comment 5

[Rail Aliiev [:rail]]

Reading the comment from https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=741751&attachment=612493 is easier then here :/

Comment 6

[Rail Aliiev [:rail]]

BTW, real repacks of installers add extra overhead which almost doubles the time needed to run partner repacks: shortest is linux (~35min vs 25min) and longest is mac (~2h vs 1h).

Comment 7

[Rail Aliiev [:rail]]

(In reply to Rail Aliiev [:rail] from comment #6)
> BTW, real repacks of installers add extra overhead which almost doubles the
> time needed to run partner repacks: shortest is linux (~35min vs 25min) and
> longest is mac (~2h vs 1h).

What doesn't really make sense because the procedure has changed only for win32 repacks, so probably connectivity is one of the issues of this increase.

Comment 8

[Rail Aliiev [:rail]]

Attachment: pass hgroot/repo

Pass hgroot/repo, it properly works in staging.

Comment 9

[Chris Cooper [:coop] (he/him)]

Comment on attachment 612493 [details] [diff] [review]
repack win32 for reals

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

Love the cleanup.

Two things before this lands. Can you please:

1) Tag the current version that does work with 3.0 and 3.6 (e.g. FIREFOX_3_6_PRODUCTION or somesuch);, and
2) Verify with Tomcat/kev that the new version of the script still works for their BYOB needs since they use the same script.

Comment 10

[Rail Aliiev [:rail]]

(In reply to Chris Cooper [:coop] from comment #9)
> 1) Tag the current version that does work with 3.0 and 3.6 (e.g.
> FIREFOX_3_6_PRODUCTION or somesuch);, and

Makes sense.

> 2) Verify with Tomcat/kev that the new version of the script still works for
> their BYOB needs since they use the same script.

Tomcat/kev, can you verify this? Or I can do this, just need instructions.

Comment 11

[Rail Aliiev [:rail]]

Comment on attachment 612550 [details] [diff] [review]
pass hgroot/repo

http://hg.mozilla.org/build/buildbotcustom/rev/2558c57ddca5

Comment 12

[Rail Aliiev [:rail]]

Comment on attachment 612493 [details] [diff] [review]
repack win32 for reals

http://hg.mozilla.org/build/partner-repacks/rev/e792d5632205

If we want to use or branch the old code for some reason (3.6?) we can use the following tag:
http://hg.mozilla.org/build/partner-repacks/rev/dd7c5bea2281 Added tag FIREFOX_3_6_PRODUCTION for changeset 6edb94da1d73

Comment 13

[Rail Aliiev [:rail]]

Kev, Tomcat

The only remaining issue here is the existing repacks. Do we need to re-repack them and sign properly or we can close the bug and wait for 12.0?

Comment 14

[Rail Aliiev [:rail]]

Please reopen if we want to re-repack Firefox 11.0 partner repacks

Comment 15

[Chris AtLee [:catlee]]

This went live in today's reconfig.

Comment 16

[Kohei Yoshino [:kohei]]

Firefox 12 looks good.
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/12.0-candidates/build1/partner-repacks/