Closed
Bug 613320
Opened 14 years ago
Closed 14 years ago
signing scripts aren't signing MARs correctly
Categories
(Release Engineering :: General, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bhearsum, Assigned: salbiz)
References
Details
(Whiteboard: [signing])
Attachments
(6 files, 7 obsolete files)
(deleted),
patch
|
bhearsum
:
review+
bhearsum
:
feedback+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
catlee
:
feedback+
bhearsum
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bhearsum
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
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
Updated•14 years ago
|
Whiteboard: [signing]
Assignee | ||
Comment 1•14 years ago
|
||
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
Attachment #491723 -
Flags: feedback?(catlee)
Attachment #491723 -
Flags: feedback?(bhearsum)
Reporter | ||
Comment 2•14 years ago
|
||
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.
Attachment #491723 -
Flags: feedback?(bhearsum) → feedback+
Reporter | ||
Comment 3•14 years ago
|
||
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.
Attachment #491723 -
Flags: review+
Assignee | ||
Comment 4•14 years ago
|
||
pending staging results
Attachment #492007 -
Flags: review?(bhearsum)
Reporter | ||
Comment 5•14 years ago
|
||
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.
Attachment #492007 -
Flags: review?(bhearsum) → review-
Reporter | ||
Comment 6•14 years ago
|
||
Comment on attachment 491723 [details] [diff] [review]
fix sorting placement for caching
changeset: 1280:21162d0c86b4
Attachment #491723 -
Flags: checked-in+
Assignee | ||
Comment 7•14 years ago
|
||
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.
Attachment #492007 -
Attachment is obsolete: true
Attachment #492445 -
Flags: feedback?(catlee)
Attachment #492445 -
Flags: feedback?(bhearsum)
Reporter | ||
Comment 8•14 years ago
|
||
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?
Assignee | ||
Comment 9•14 years ago
|
||
sort order after patch
Assignee | ||
Comment 10•14 years ago
|
||
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
Assignee | ||
Comment 11•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 491723 [details] [diff] [review]
fix sorting placement for caching
You're re-doing this in the later patch, right?
Attachment #491723 -
Flags: feedback?(catlee)
Comment 13•14 years ago
|
||
Comment on attachment 492445 [details] [diff] [review]
tweak sort order for fileKey
Looks good.
Did you write some unittests for this?
Attachment #492445 -
Flags: feedback?(catlee) → feedback+
Reporter | ||
Comment 14•14 years ago
|
||
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.
Attachment #492445 -
Flags: feedback?(bhearsum) → feedback+
Assignee | ||
Comment 15•14 years ago
|
||
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.
Attachment #492851 -
Flags: feedback?(catlee)
Attachment #492851 -
Flags: feedback?(bhearsum)
Reporter | ||
Updated•14 years ago
|
Priority: -- → P2
Reporter | ||
Comment 16•14 years ago
|
||
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.
Attachment #492851 -
Flags: feedback?(bhearsum) → feedback-
Comment 17•14 years ago
|
||
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?
Attachment #492851 -
Flags: feedback?(catlee) → feedback-
Assignee | ||
Comment 18•14 years ago
|
||
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.
Attachment #492851 -
Attachment is obsolete: true
Attachment #493275 -
Flags: feedback?(catlee)
Attachment #493275 -
Flags: feedback?(bhearsum)
Assignee | ||
Comment 19•14 years ago
|
||
d'oh, forgot to refresh patch before submitting
Attachment #493275 -
Attachment is obsolete: true
Attachment #493276 -
Flags: feedback?(catlee)
Attachment #493276 -
Flags: feedback?(bhearsum)
Attachment #493275 -
Flags: feedback?(catlee)
Attachment #493275 -
Flags: feedback?(bhearsum)
Reporter | ||
Comment 20•14 years ago
|
||
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.
Attachment #493276 -
Flags: feedback?(bhearsum) → feedback+
Assignee | ||
Comment 21•14 years ago
|
||
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.
Attachment #493276 -
Attachment is obsolete: true
Attachment #493294 -
Flags: feedback?(catlee)
Attachment #493294 -
Flags: feedback?(bhearsum)
Attachment #493276 -
Flags: feedback?(catlee)
Reporter | ||
Comment 22•14 years ago
|
||
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.
Attachment #493294 -
Flags: feedback?(bhearsum) → feedback+
Reporter | ||
Comment 23•14 years ago
|
||
Comment on attachment 493294 [details] [diff] [review]
refactor signing v4
Builds signed with this patch had no issues with updates. Adjusting to r+.
Attachment #493294 -
Flags: review+
Assignee | ||
Comment 24•14 years ago
|
||
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.
Attachment #493294 -
Attachment is obsolete: true
Attachment #493696 -
Flags: review?(catlee)
Attachment #493696 -
Flags: review?(bhearsum)
Attachment #493294 -
Flags: feedback?(catlee)
Updated•14 years ago
|
Attachment #493696 -
Flags: review?(catlee) → review+
Assignee | ||
Comment 25•14 years ago
|
||
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
Attachment #493696 -
Attachment is obsolete: true
Attachment #493742 -
Flags: review?(bhearsum)
Attachment #493696 -
Flags: review?(bhearsum)
Reporter | ||
Updated•14 years ago
|
Attachment #493742 -
Flags: review?(bhearsum) → review+
Assignee | ||
Comment 26•14 years ago
|
||
d'oh, patch wasn't refreshed against the tree, failed to apply cleanly. No major changes required to make things right.
Attachment #493742 -
Attachment is obsolete: true
Attachment #493749 -
Flags: review?(bhearsum)
Reporter | ||
Comment 27•14 years ago
|
||
Comment on attachment 493749 [details] [diff] [review]
refresh patch
changeset: 1322:de9a853c8856
Attachment #493749 -
Flags: review?(bhearsum)
Attachment #493749 -
Flags: review+
Attachment #493749 -
Flags: checked-in+
Reporter | ||
Comment 28•14 years ago
|
||
We should be all done here now.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•