rustbuild: Verify sha256 of downloaded tarballs #32926

caipre · 2016-04-13T03:56:31Z

Here's a quick first pass at this.

I don't use Python often enough to claim that this is totally Pythonic. I've left off some (almost certainly unnecessary) error handling regarding opening and processing files. The whole tarball is read into memory to calculate the hash, but the file isn't so large so that should be fine. I don't care for the output from raise RuntimeError, but that's how run() does it so I'm following precedent.

Tested by manually changing the value of expected, and by modifying the tarball then forcing rustc_out_of_date(). Both cases tripped the error.

Closes #32902

rust-highfive · 2016-04-13T03:56:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

caipre · 2016-04-13T03:57:58Z

r? @alexcrichton

mitaa · 2016-04-13T04:40:31Z

src/bootstrap/bootstrap.py

+        if sys.platform == 'win32':
+            run(["PowerShell.exe", "/nologo", "-Command",
+                "(New-Object System.Net.WebClient).DownloadFile('" + url +
+                    "', '" + path + "')"], verbose=verbose)


While you're here I think this would be much more readable as

"(New-Object System.Net.WebClient).DownloadFile('{}', '{}')".format(url, path)

(I think .format is more idiomatic than str + foo anyway)

alexcrichton · 2016-04-13T06:10:16Z

Thanks @caipre! Your thoughts on #32902 (comment) were actually spot on, I was thinking of committing the hash into the source tree itself (e.g. a line in src/nightlies.txt). That way we don't have to worry about the sha accidentally changing in transit as well!

alexcrichton · 2016-04-13T06:11:50Z

Also while you're at it, wanna take care of #32834 as well? It would basically involve downloading into .foo.tar.gz and then only renaming it to foo.tar.gz once the download has finished and the checksum is verified.

mitaa · 2016-04-13T06:21:50Z

src/bootstrap/bootstrap.py

+    with open(path, "rb") as f:
+        found = hashlib.sha256(f.read()).hexdigest()
+    if found != expected:
+        if not verbose:


if verbose:? Or just put it in raise RuntimeError("invalid checksum: {}".format(local_sum))?

Where is local_sum defined? Should probably be found?

Good eye, thanks. I must've missed renaming this one.

I think it makes sense to put this RuntimeError and the one in run() behind the verbose flag.

caipre · 2016-04-13T14:24:45Z

@alexcrichton: Okay, I can do it that way. It's a bit ugly though, since we'll have to put the shas of all the different build triples for the various tarballs into the src/nightlies.txt file. We'll probably want to autogenerate that file.

How is the snapshot build decided upon? Is there a parameter set as part of some build script when a new snapshot is selected?

Regarding #32834, looks like @cyplo is actively working on it, so I'll let them run with it for now.

cyplo · 2016-04-13T14:31:29Z

Hi @caipre ! Shall we sync the development here in some way ? e.g. me branching off these changes instead of master or waiting for the sha check first ? Or the other way round, shall I try to make the change quickly so you can add the sha check there ? Let me know how would you like for this to work. Thanks !

caipre · 2016-04-13T15:09:20Z

If there are merge conflicts they shouldn't be too hairy, so I don't think we need to coordinate our work too much. If mine lands first, I'm happy to help you resolve conflicts if necessary.

Don't branch off this commit as I won't be using it per the comments from Alex.

alexcrichton · 2016-04-13T16:22:32Z

Gah right, sorry @cyplo and @caipre! Disregard me :) I agree though that the merge conflicts here, although they'll probably exist, should be easy to fix. This is essentially just verifying after the download, so whatever logic we have for downloading will fit well above it.

It's a bit ugly though, since we'll have to put the shas of all the different build triples for the various tarballs into the src/nightlies.txt file. We'll probably want to autogenerate that file.

Hm yeah, that's a good point. @brson do you have thoughts on this? Should we check the sha256 in the repo or just download it like rustup and verify both downloads?

How is the snapshot build decided upon?

Right now it's just the date in src/nightlies.txt, and we'll update that periodically

caipre · 2016-04-13T17:15:18Z

I was more asking why/how the date is chosen. Why 2016-03-20 vs the latest nightly/beta/stable?

Maybe I'll take a look at the logic for which stage0 compiler is downloaded.

alexcrichton · 2016-04-13T17:40:15Z

Oh that's actually relatively arbitrary right now. Starting very soon though it will become the previous release. (e.g. 1.10 will bootstrap from 1.9)

alexcrichton · 2016-04-13T18:16:49Z

Ok, checked in with @brson on IRC and he thinks we should just download the sha256, so let's stick with that strategy

alexcrichton · 2016-04-13T18:17:49Z

Ok, apart from the comments by @mitaa, could you also fold this directly into the get function? That way we don't have to remember to call verify later on

caipre · 2016-04-13T19:45:35Z

I originally had it folded into get(), but that prevents any verification if we don't do a download, which was how I interpreted your second point: "Make sure the file itself wasn't tampered with."

alexcrichton · 2016-04-13T20:13:41Z

Oh sorry that was just intended for the in-transit file. Once we've downloaded and verified there's no need to verify it again I believe

For normal invocations, print a short error message and exit. When the verbose option is enabled, also print the backtrace.

caipre · 2016-04-14T02:15:14Z

@alexcrichton: Okay, I've pushed new commits that address the comments so far. Thanks!

alexcrichton · 2016-04-14T06:59:11Z

@bors: r+ e0f997d

Thanks!

bors · 2016-04-14T19:22:30Z

⌛ Testing commit e0f997d with merge ed08f48...

bors · 2016-04-14T19:24:36Z

💔 Test failed - auto-win-msvc-64-cargotest

alexcrichton · 2016-04-14T20:08:25Z

@bors: retry

On Thu, Apr 14, 2016 at 12:24 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-msvc-64-cargotest
http://buildbot.rust-lang.org/builders/auto-win-msvc-64-cargotest/builds/67

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#32926 (comment)

…=alexcrichton rustbuild: Verify sha256 of downloaded tarballs Here's a quick first pass at this. I don't use Python often enough to claim that this is totally Pythonic. I've left off some (almost certainly unnecessary) error handling regarding opening and processing files. The whole tarball is read into memory to calculate the hash, but the file isn't *so* large so that should be fine. I don't care for the output from `raise RuntimeError`, but that's how `run()` does it so I'm following precedent. Tested by manually changing the value of `expected`, and by modifying the tarball then forcing `rustc_out_of_date()`. Both cases tripped the error. Closes rust-lang#32902

Rollup of 11 pull requests - Successful merges: #32923, #32926, #32929, #32931, #32935, #32945, #32946, #32964, #32970, #32973, #32997 - Failed merges:

rust-highfive assigned aturon Apr 13, 2016

rust-highfive assigned alexcrichton and unassigned aturon Apr 13, 2016

mitaa reviewed Apr 13, 2016
View reviewed changes

caipre added 2 commits April 13, 2016 22:10

rustbuild: Improve error messaging in bootstrap.py

ffff91a

For normal invocations, print a short error message and exit. When the verbose option is enabled, also print the backtrace.

rustbuild: Verify sha256 of downloaded tarballs

e0f997d

alexcrichton mentioned this pull request Apr 14, 2016

Tracking issue for rustbuild, the alternate build system for rustc #31590

Closed

48 tasks

Manishearth mentioned this pull request Apr 15, 2016

Rollup of 11 pull requests #33005

Merged

bors added a commit that referenced this pull request Apr 15, 2016

Auto merge of #33005 - Manishearth:rollup, r=Manishearth

576229f

Rollup of 11 pull requests - Successful merges: #32923, #32926, #32929, #32931, #32935, #32945, #32946, #32964, #32970, #32973, #32997 - Failed merges:

bors merged commit e0f997d into rust-lang:master Apr 16, 2016

caipre deleted the rustbuild-verify-download branch April 16, 2016 00:51

cyplo mentioned this pull request Apr 16, 2016

rustbuild seems to deal badly with poor internet connections #32834

Closed

alexcrichton mentioned this pull request Jul 5, 2016

Validate gpg signatures of downloaded nightlies #34598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustbuild: Verify sha256 of downloaded tarballs #32926

rustbuild: Verify sha256 of downloaded tarballs #32926

caipre commented Apr 13, 2016

rust-highfive commented Apr 13, 2016

caipre commented Apr 13, 2016

mitaa Apr 13, 2016

alexcrichton commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

mitaa Apr 13, 2016

caipre Apr 13, 2016

caipre commented Apr 13, 2016

cyplo commented Apr 13, 2016

caipre commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

caipre commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

caipre commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

caipre commented Apr 14, 2016

alexcrichton commented Apr 14, 2016

bors commented Apr 14, 2016

bors commented Apr 14, 2016

alexcrichton commented Apr 14, 2016

rustbuild: Verify sha256 of downloaded tarballs #32926

rustbuild: Verify sha256 of downloaded tarballs #32926

Conversation

caipre commented Apr 13, 2016

rust-highfive commented Apr 13, 2016

caipre commented Apr 13, 2016

mitaa Apr 13, 2016

Choose a reason for hiding this comment

alexcrichton commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

mitaa Apr 13, 2016

Choose a reason for hiding this comment

caipre Apr 13, 2016

Choose a reason for hiding this comment

caipre commented Apr 13, 2016

cyplo commented Apr 13, 2016

caipre commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

caipre commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

caipre commented Apr 13, 2016

alexcrichton commented Apr 13, 2016

caipre commented Apr 14, 2016

alexcrichton commented Apr 14, 2016

bors commented Apr 14, 2016

bors commented Apr 14, 2016

alexcrichton commented Apr 14, 2016