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)

defect
Not set
normal

Tracking

(firefox52 wontfix, firefox53 fixed, firefox54 fixed)

RESOLVED FIXED
Tracking Status
firefox52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: rbarnes, Assigned: rail)

References

Details

Attachments

(2 files)

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 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 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-
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.
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 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 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+
Attachment #8841638 - Flags: review?(stephouillon)
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+
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
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?
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee: nobody → rlb
Blocks: 1341396
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+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: rlb → rail
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?
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 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 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+
Keywords: leave-open
Pushed by raliiev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1deede261ebf Upload Merkle Tree summary r=jlund a=release DONTBUILD
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.

Attachment

General

Created:
Updated:
Size: