Closed
Bug 1342974
Opened 8 years ago
Closed 8 years ago
Generate a Merkle Tree to summarize the realease
Categories
(Release Engineering :: Release Automation, defect)
Release Engineering
Release Automation
Tracking
(firefox52 wontfix, firefox53 fixed, firefox54 fixed)
RESOLVED
FIXED
People
(Reporter: rbarnes, Assigned: rail)
References
Details
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
arroway
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
jlund
:
review+
|
Details |
For binary transparency, we need a single hash value that represents a release and an efficient way to tie any given binary to that hash value. A Merkle tree on top of the existing SHAXXXSUMS files provides these things.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 2•8 years ago
|
||
Comment on attachment 8841638 [details]
Bug 1342974 - Add code to generate a Merkle tree summary of a release
This patch augments generate-checksums.py so that it also builds a Merkle tree on the checksums it finds. It then writes a SHAXXXSUMMARY file containing (1) the tree head and (2) an inclusion proof for each file.
rail, catlee: Does this look like the right direction to be headed? Review ready?
Attachment #8841638 -
Flags: feedback?(rail)
Attachment #8841638 -
Flags: feedback?(catlee)
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8841638 [details]
Bug 1342974 - Add code to generate a Merkle tree summary of a release
https://reviewboard.mozilla.org/r/115796/#review117184
Looks good overall! As mentioned below, I think this could benefit from being split out into its own file, and definitely could use some tests given that I don't think the code as written will actually run.
::: testing/mozharness/scripts/release/generate-checksums.py:18
(Diff revision 1)
> from mozharness.base.vcs.vcsbase import VCSMixin
> from mozharness.mozilla.checksums import parse_checksums_file
> from mozharness.mozilla.signing import SigningMixin
> from mozharness.mozilla.buildbot import BuildbotMixin
>
> +class MerkleTree:
having this broken out into its own file, with some tests, would probably be clearer.
::: testing/mozharness/scripts/release/generate-checksums.py:34
(Diff revision 1)
> + self.hash_fn = hash_fn
> +
> + self.nodes = self.n * [None]
> + for i in range(self.n):
> + self.nodes[i] = self.n * [None]
> + self.nodes[i][0] = self.leafHash(data[i])
this data structure is a little opaque. can you document what you are trying to do here? would a dict work better? or memoizing intermediate node calculations for the inclusion proof path generation?
::: testing/mozharness/scripts/release/generate-checksums.py:36
(Diff revision 1)
> + self.nodes = self.n * [None]
> + for i in range(self.n):
> + self.nodes[i] = self.n * [None]
> + self.nodes[i][0] = self.leafHash(data[i])
> +
> + def round2(n):
this looks like it's intended to be a regular function (outside of a class), not an instance method.
::: testing/mozharness/scripts/release/generate-checksums.py:52
(Diff revision 1)
> +
> + def node(self, start, end):
> + if self.nodes[start][end-start-1] != None:
> + return self.nodes[start][end-start-1]
> +
> + k = round2(end - start)
this won't work unless called via self.round2, or if round2 is moved outside of the class
Attachment #8841638 -
Flags: review-
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8841638 [details]
Bug 1342974 - Add code to generate a Merkle tree summary of a release
https://reviewboard.mozilla.org/r/115796/#review117176
::: testing/mozharness/scripts/release/generate-checksums.py:45
(Diff revision 1)
> + return k >> 1
> +
> + def leafHash(self, a):
> + return self.hash_fn('\x00' + a).digest()
> +
> + def pairHash(self, a, b):
I'd name the methods using the same approach, and rename this one to `pair_hash()` to be consistent with others.
::: testing/mozharness/scripts/release/generate-checksums.py:248
(Diff revision 1)
> + if format_ == "sha256":
> + return hashlib.sha256
> + elif format_ == "sha384":
> + return hashlib.sha384
> + elif format_ == "sha512":
> + return hashlib.sha512
You can probably "optimize" this as
```python
if format_ in ("sha256", "sha384", "sha512"):
return getattr(hashlib, format_)
else:
...
```
but meh :)
::: testing/mozharness/scripts/release/generate-checksums.py:316
(Diff revision 1)
> self.checksums[f] = info
> break
> else:
> self.debug("Ignoring checksums for file: {}".format(f))
>
> + def compute_summary_info(self):
This action should be added to `all_actions`, so we can call the script with `--compute-summary-info`
::: testing/mozharness/scripts/release/generate-checksums.py:333
(Diff revision 1)
> + head = tree.head().encode("hex")
> + proofs = [tree.inclusion_proof(i).encode("hex") for i in range(len(files))]
> +
> + summary = self._get_summary_filename(fmt)
> + self.info("Creating summary file: {}".format(summary))
> + with open(sums, "w+") as output_file:
You can use `self.write_to_file()` (see https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#902) instead. Don't ask me why the following function uses `with open` :), probably we ported the script from another repo.
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8841638 [details]
Bug 1342974 - Add code to generate a Merkle tree summary of a release
In overall it looks good, just need to address some comments and test. Thank you!
Attachment #8841638 -
Flags: feedback?(rail) → feedback+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8841638 [details]
Bug 1342974 - Add code to generate a Merkle tree summary of a release
Updated version:
- Factors out Merkle tree computations to a separate file
- Adds unit tests for Merkle tree operations in testing/mozharness/tests
- Uses a more pythonic caching structure (I think?)
- names_with_underscores
- write_to_file
Attachment #8841638 -
Flags: review?(rail)
Attachment #8841638 -
Flags: review?(catlee)
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8841638 [details]
Bug 1342974 - Add code to generate a Merkle tree summary of a release
https://reviewboard.mozilla.org/r/115796/#review117570
Attachment #8841638 -
Flags: review?(catlee) → review+
Comment hidden (mozreview-request) |
Reporter | ||
Updated•8 years ago
|
Attachment #8841638 -
Flags: review?(stephouillon)
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8841638 [details]
Bug 1342974 - Add code to generate a Merkle tree summary of a release
https://reviewboard.mozilla.org/r/115796/#review118322
::: testing/mozharness/mozharness/mozilla/merkle.py:41
(Diff revision 4)
> + - 8 octets of leaf_index = m
> + - 2 octets of path length, followed by
> + * 1 + N octets of NodeHash
> + """
> +
> + # Pre-generated 'log ID'. Not used by Firefox; it is only needed becuase
"becuase" -> "because"
Attachment #8841638 -
Flags: review?(stephouillon) → review+
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
Pushed by rlb@ipv.sx:
https://hg.mozilla.org/integration/autoland/rev/dac6db07aabb
Add code to generate a Merkle tree summary of a release r=arroway,catlee
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8841638 [details]
Bug 1342974 - Add code to generate a Merkle tree summary of a release
Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Unnecessary delay; this code is not run for Nightly or Aurora, so there's no point to having it wait there for two cycles
[Is this code covered by automated tests?]: Yes; see tests included in patch
[Has the fix been verified in Nightly?]: N/A; this code is not run for Nightly or Aurora
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: The only code this change affects is code for generating release metadata. The worst possible impact is that it could delay shipping a beta release.
[String changes made/needed]: None
Attachment #8841638 -
Flags: approval-mozilla-beta?
Comment 15•8 years ago
|
||
bugherder |
Updated•8 years ago
|
status-firefox52:
--- → wontfix
status-firefox53:
--- → affected
Comment 16•8 years ago
|
||
Comment on attachment 8841638 [details]
Bug 1342974 - Add code to generate a Merkle tree summary of a release
Context: https://wiki.mozilla.org/Security/Binary_Transparency
This should land for the beta 2 build next Monday.
Attachment #8841638 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•8 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 18•8 years ago
|
||
we need to add a line here: https://hg.mozilla.org/releases/mozilla-beta/file/f4c59968a6ea/testing/mozharness/scripts/release/generate-checksums.py#l294 and call _get_summary_filename() to upload the files.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•8 years ago
|
Assignee: rlb → rail
Comment hidden (mozreview-request) |
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8847686 [details]
Bug 1342974 - Upload Merkle Tree summary a=release DONTBUILD
https://reviewboard.mozilla.org/r/120602/#review123110
::: testing/mozharness/scripts/release/generate-checksums.py:296
(Diff revision 1)
> files = ['KEY']
>
> for fmt in self.config["formats"]:
> files.append(self._get_sums_filename(fmt))
> files.append("{}.asc".format(self._get_sums_filename(fmt)))
> + files.append(self._get_summary_filename(fmt))
having _get_summary_filename and _get_sums_filename seems wrong. Should we rename method name?
Assignee | ||
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8847686 [details]
Bug 1342974 - Upload Merkle Tree summary a=release DONTBUILD
https://reviewboard.mozilla.org/r/120602/#review123886
::: testing/mozharness/scripts/release/generate-checksums.py:296
(Diff revision 1)
> files = ['KEY']
>
> for fmt in self.config["formats"]:
> files.append(self._get_sums_filename(fmt))
> files.append("{}.asc".format(self._get_sums_filename(fmt)))
> + files.append(self._get_summary_filename(fmt))
Can you elaborate on this? Do you find both names conflicting and confusing?
Comment 22•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8847686 [details]
Bug 1342974 - Upload Merkle Tree summary a=release DONTBUILD
https://reviewboard.mozilla.org/r/120602/#review123886
> Can you elaborate on this? Do you find both names conflicting and confusing?
sorry, should have been more explicit. Yes I initially found them conflicting. However, rereading, I suppose 'sums' vs 'summary' isn't so bad.
I won't push back if you think it's fine.
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8847686 [details]
Bug 1342974 - Upload Merkle Tree summary a=release DONTBUILD
https://reviewboard.mozilla.org/r/120602/#review124640
Attachment #8847686 -
Flags: review?(jlund) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 24•8 years ago
|
||
Pushed by raliiev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1deede261ebf
Upload Merkle Tree summary r=jlund a=release DONTBUILD
Comment 25•8 years ago
|
||
bugherder |
Assignee | ||
Comment 26•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/02747e813bf00e7d2d862f30129c26088463b7f1
https://hg.mozilla.org/releases/mozilla-beta/rev/44f9af6ad433e022479f2e32fb499036ee1440d5
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 27•7 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•