613320 - signing scripts aren't signing MARs correctly

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[[e-mail] (:bhearsum)]

In my 3.6.13 staging run I found that the signing scripts aren't sharing signed binaries between installers and MARs. The internal binaries across all of the locales match if you only look at installers or only look at MARs, but they're supposed to be consistent across the board:

1c52a00cc4feb90187ee9daed82b49c7  ./af/exe/nonlocalized/firefox.exe
49ed298a99bb51c07a0dbec88878044d  ./af/mar/firefox.exe.out
1c52a00cc4feb90187ee9daed82b49c7  ./en-US/exe/nonlocalized/firefox.exe
49ed298a99bb51c07a0dbec88878044d  ./en-US/mar/firefox.exe.out

Comment 1

[Syed Albiz [:salbiz]]

Attachment: fix sorting placement for caching

so, this went away with remember=False explicitly set, and I found that the leading path for mars for non-partner repacks was getting set to None instead of an empty string. This caused mars to be sorted ahead of exes in the caching, which screwed up how caching dealt with them (since the script assumes mars get signed after exes). I've tested this with the 7 locales used for ffx-3.6.13, will do a complete run with all locales before prompting for review

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 491723 [details] [diff] [review]
fix sorting placement for caching

Seems good to me. I'll give this a try in my staging run today.

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 491723 [details] [diff] [review]
fix sorting placement for caching

This patch works in my tests but still sorts the first en-US partner repack ahead of the plain en-US build. Catlee suggested that this is because changing from None to '' doesn't affect the sort order. If we don't get this fixed before the releases this will be OK, but we need to fix the sort order soon.

Comment 4

[Syed Albiz [:salbiz]]

Attachment: amend regex to catch all partner repacks with dashes in the name

pending staging results

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 492007 [details] [diff] [review]
amend regex to catch all partner repacks with dashes in the name

I ran update verify against the builds signed with this patch and ended up with inconsistent internal signatures. We'll have to go with the first patch for the releases on Monday.

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 491723 [details] [diff] [review]
fix sorting placement for caching

changeset:   1280:21162d0c86b4

Comment 7

[Syed Albiz [:salbiz]]

Attachment: tweak sort order for fileKey

This amends the sort order such that the files are signed (plain exes; partner-repack exes; complete mars). In my local tests this reliably works with all locales and partner-repacks, although I have not run update-verify on signed builds. In future, I want to refactor this functionality to catch these errors earlier through unit tests. Running it in staging now, will ask for review pending results.

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 492445 [details] [diff] [review]
tweak sort order for fileKey

Any chance you could attach a list of the sort order before and after this patch?

Comment 9

[Syed Albiz [:salbiz]]

Attachment: sort order (after patch)

sort order after patch

Comment 10

[Syed Albiz [:salbiz]]

Attachment: sort order (with patch to fix regex)

This fixes the regex (as per the previous patch), but leaves the fileKey function untouched, causing partner-repack exes to be sorted after plain locale MARs

Comment 11

[Syed Albiz [:salbiz]]

Attachment: sort order (before all patches)

This is the sort order produced with the faulty regex, where some partner repacks are mistakenly identified with an empty leading path due to the presence of non-word class characters ('-' seems to be the primary offender). Hence we get partner-repacks being signed before plain locale repacks since to the sort function they are equal in all aspects except the actual filename.

Comment 12

[Chris AtLee [:catlee]]

Comment on attachment 491723 [details] [diff] [review]
fix sorting placement for caching

You're re-doing this in the later patch, right?

Comment 13

[Chris AtLee [:catlee]]

Comment on attachment 492445 [details] [diff] [review]
tweak sort order for fileKey

Looks good.

Did you write some unittests for this?

Comment 14

[[e-mail] (:bhearsum)]

Comment on attachment 492445 [details] [diff] [review]
tweak sort order for fileKey

I'd like to see the results of a staging run that has some locales and partner builds in it. You can probably save a lot of pain by using the unsigned builds from my 3.6.13 staging and starting at signing, and then running updates + update_verify.

Comment 15

[Syed Albiz [:salbiz]]

Attachment: refactor signing

This is the approach I've been working on for the past few days, it makes a couple of big changes to the signing scripts:

-factors out the logic to sort and filter files that need signing into a separate function. Based on my testing, I cannot find anything wrong with the binaries produced by the initial sort order (firstLocale, leadingPath, locale, exeVal, f), so I have used that precedence ordering in this patch.

-adds a unit-test for this new function to make sure that the sort order applies as expected

-adds a new target to verify that the contents of the MARs and installers have the same checksum.

Tested on the staging box, but I have not yet run updates/update_verify on the signed builds.

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 492851 [details] [diff] [review]
refactor signing

It looks like verify-checksums.py could be integrated with verify-signatures.py without too much trouble. Can you look at doing that? It'd save us a lot of unpacking time.

Hmm, I like that the filtering is in a function but I think you should separate filtering them and sorting them.

The details of the filtering, sorting, and test look fine.

Comment 17

[Chris AtLee [:catlee]]

Comment on attachment 492851 [details] [diff] [review]
refactor signing

>diff --git a/release/signing/verify_checksums.py b/release/signing/verify_checksums.py
>new file mode 100755
>--- /dev/null
>+++ b/release/signing/verify_checksums.py
>@@ -0,0 +1,91 @@
>+#!/usr/bin/env python
>+"""verify-checksums"""
>+import tempfile, shutil, sys, os
>+from signing import unpackfile, sha1sum, findfiles, \
>+shouldSign, bunzip2
>+import logging
>+LOG = logging.getLogger()

we've been using 'log' in lots of other places as the module-level logger,
let's keep that consistent.

>+def get_checksums(source):
>+    """return a dict of sha1 checksums for all binaries in a container archive
>+    file"""
>+    tmpdir = tempfile.mkdtemp()
>+    LOG.info("Copying files into %s" % tmpdir)
>+    shutil.copy(source, tmpdir)
>+    unpackfile(source, tmpdir)
>+    checksums = {}
>+    for signed_file in findfiles(tmpdir):
>+        if shouldSign(signed_file):
>+            if source.endswith(".mar"):
>+                bunzip2(signed_file)
>+            checksums[os.path.basename(signed_file)] = sha1sum(signed_file)
>+    return checksums
>+
>+def sums_are_equal(inst_sums, update_sums):
>+    """check to make sure that the two dictionaries of files:checksums are
>+    exactly equal"""
>+    equality = True
>+    for signed_file in update_sums.keys():
>+        if inst_sums[signed_file] != update_sums[signed_file]:
>+            equality = False
>+    return equality

This will miss cases where inst_sums contains files not in update_sums...is that ok?

Comment 18

[Syed Albiz [:salbiz]]

Attachment: refactor signing v2

Following changes implemented:

* Merged verify-checksums.py functionality into verify-signature.py, which saves us a nice chunk of unpacking time.

* Separated sorting/filtering functions. Not sure if this really gives us much other than testability, since for the sorting key function to apply predictably and correctly, the file list needs to be filtered, added separate test for the filtering functionality

* Added a quick-verify target that only verifies en-US, one randomly selected locale and one randomly selected partner repack to verify prior to upload as part of the post-sign target.

The reason I'm matching against the mar's list of files is because the installer contains a bunch of signed binaries that aren't in the mar (setup.exe), and the unpacking file checks in verify-signature.py already check for inconsistent files in signing.

Comment 19

[Syed Albiz [:salbiz]]

Attachment: refactor signing v3

d'oh, forgot to refresh patch before submitting

Comment 20

[[e-mail] (:bhearsum)]

Comment on attachment 493276 [details] [diff] [review]
refactor signing v3

Hmmm, why did you move quick-verify to the postsign target?

It would be good to be a bit more realistic in test_filtering() by using realistic filenames:
unsigned/linux-i686/en-US/firefox-3.6.13.tar.bz2
unsigned/win32/en-US/Firefox Setup 3.6.13.exe
unsigned/win32/en-US/firefox-3.6.13.zip

I think quick verify should do slighty more than what it is, specifically it should test:
- plain en-US installer
- plain en-US mar
- en-US partner repack installer
- locale installer
- locale mar
- locale partner repack

Where 'locale' is consistent.

Can you add a comment documenting how the shared dict references interact? It's not transparent to me.

Comment 21

[Syed Albiz [:salbiz]]

Attachment: refactor signing v4

Done, the quick-verify was already testing all of the cases save for testing an additional partner-repack for the l10n repack.

I've also moved the test cases into the already existing tests.py, and updated the existing test cases. I kept one of the probably unrealistic cases where we have an exe file under a linux directory just to ensure that filtering py platform takes precedence over filtering by file extension. I've tried to document my approach to the checksum stuff a bit more, but if it still doesn't seem understandable then I should probably just refactor it somehow.

Comment 22

[[e-mail] (:bhearsum)]

Comment on attachment 493294 [details] [diff] [review]
refactor signing v4

LGTM now. I'd like to see results of signing, verify signatures, and update verify prior to landing though.

Comment 23

[[e-mail] (:bhearsum)]

Comment on attachment 493294 [details] [diff] [review]
refactor signing v4

Builds signed with this patch had no issues with updates. Adjusting to r+.

Comment 24

[Syed Albiz [:salbiz]]

Attachment: more verbose logging, fix docstrings, fix checksum_verify and add more test coverage

Implemented the suggestions that came up during the testing run. These are all limited to touching only the verification step, so I've tested with the unit test suite and a quick verify on known-good builds. Starting a complete verification run now, will update with results when complete.

Comment 25

[Syed Albiz [:salbiz]]

Attachment: make logging output more readable

Just changing logging lines to enumerate all packages being compared on a new line. Nothing else touched, so I've just run a quick-verify to sanity check

Comment 26

[Syed Albiz [:salbiz]]

Attachment: refresh patch

d'oh, patch wasn't refreshed against the tree, failed to apply cleanly. No major changes required to make things right.

Comment 27

[[e-mail] (:bhearsum)]

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

changeset:   1322:de9a853c8856

Comment 28

[[e-mail] (:bhearsum)]

We should be all done here now.