Closed Bug 1342974 Opened 3 years ago Closed 3 years ago

Generate a Merkle Tree to summarize the realease

Categories

(Release Engineering :: Release Automation: Other, defect)

Product:
Release Engineering
Component:
Release Automation: Other
Type:
defect
Priority:
Not set

Tracking

(firefox52 wontfix, firefox53 fixed, firefox54 fixed)

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

People

(Reporter: rbarnes, Assigned: rail)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1342974 - Add code to generate a Merkle tree summary of a release
59 bytes, text/x-review-board-request
arroway
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1342974 - Upload Merkle Tree summary a=release DONTBUILD
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 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?
https://hg.mozilla.org/mozilla-central/rev/dac6db07aabb
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox54: --- → fixed
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+
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: 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+
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.