Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[build] Verify SHA checksums of downloaded node executables #7136

Closed
ycombinator opened this issue May 4, 2016 · 10 comments
Closed

[build] Verify SHA checksums of downloaded node executables #7136

ycombinator opened this issue May 4, 2016 · 10 comments
Assignees
Labels
bug Fixes for quality problems that affect the customer experience good first issue low hanging fruit PR sent Team:Operations Team label for Operations Team

Comments

@ycombinator
Copy link
Contributor

ycombinator commented May 4, 2016

While testing #6267, @LeeDr noticed that the build ended up packaging a bad node.exe file. It turned out that the node.exe file downloaded by the build process into the .node_binaries folder was bad.

I looked at the code where the various node executables are downloaded; it appears we only check for a HTTP 200 status code to determine success. Perhaps we should also compare the SHA checksums (example for v4.3.2) of the downloaded node executables before proceeding further in the build?

@LeeDr
Copy link

LeeDr commented May 4, 2016

+1 on the P1. For whatever reason, this download is very unreliable at the moment. I'm trying multiple times and not getting a good download of node.exe on Windows.

@LeeDr
Copy link

LeeDr commented May 4, 2016

this might be a clue. As I keep retrying the build to get a good node.exe, this time I got;

Warning: Client request error: getaddrinfo ENOTFOUND nodejs.org nodejs.org:443 Use --force to continue.

@ycombinator
Copy link
Contributor Author

That looks like a DNS lookup failure, which should result in no download, not a partial download. Also, if its a DNS lookup failure, I'd expect this callback to be fired with an error. Strange.

@epixa
Copy link
Contributor

epixa commented May 4, 2016

We should definitely be checking shas.

@LeeDr
Copy link

LeeDr commented May 4, 2016

There's this method function validateDownload(path, expectedHash, success)
here;
https://github.com/elastic/kibana/blob/master/tasks/download_selenium.js#L18

@epixa
Copy link
Contributor

epixa commented May 4, 2016

To clarify, I didn't mean to imply that I think we are checking for shas. We should just start if we aren't already.

@LeeDr
Copy link

LeeDr commented May 5, 2016

Apparently wreck.request is not very robust for download (on Windows). It's almost ALWAYS failing to properly download node.exe compared to wget https://nodejs.org/dist/v4.3.2/win-x86/node.exe which just worked perfectly 5 times in a row.

@LeeDr
Copy link

LeeDr commented May 5, 2016

I'll suggest we change from using wreck.request('GET',... to request.get(URL) like we use when we download the selenium jar. I don't think I've ever had problems with that.

@bevacqua
Copy link
Contributor

Made a utility module to help with this: check-hash

@Bargs Bargs added the good first issue low hanging fruit label Jul 8, 2016
@ppisljar ppisljar self-assigned this Jul 11, 2016
@ppisljar
Copy link
Member

ppisljar commented Jul 12, 2016

@bevacqua utility only returns md5 ... we would need sha256 ... would you mind adding an option to define hashing algorithm used ? ( i submited a pull request against your repo)

@ppisljar ppisljar added WIP Work in progress and removed WIP Work in progress labels Jul 17, 2016
ppisljar added a commit to ppisljar/kibana that referenced this issue Oct 29, 2016
@epixa epixa added Team:Operations Team label for Operations Team and removed discuss labels Oct 29, 2016
@epixa epixa added bug Fixes for quality problems that affect the customer experience PR sent labels Oct 29, 2016
ppisljar added a commit that referenced this issue Oct 29, 2016
* fix #7136 - check SHA of downloaded node binaries
* skips download if --skip-node-download cli argument is present
elastic-jasper added a commit that referenced this issue Oct 29, 2016
Backports PR #7746

**Commit 1:**
fix #7136 - check SHA of downloaded node binaries

* Original sha: 955972b
* Authored by ppisljar <[email protected]> on 2016-07-11T19:17:08Z

**Commit 2:**
only skipping download if --skip-download cli argument is present

* Original sha: 325e172
* Authored by ppisljar <[email protected]> on 2016-09-07T10:54:23Z

**Commit 3:**
updating log messages based on epixas comments

* Original sha: 20b5c4d
* Authored by ppisljar <[email protected]> on 2016-09-23T06:24:55Z

**Commit 4:**
updating based on courts review

* Original sha: 78c124c
* Authored by ppisljar <[email protected]> on 2016-10-29T14:44:43Z
elastic-jasper added a commit that referenced this issue Oct 29, 2016
Backports PR #7746

**Commit 1:**
fix #7136 - check SHA of downloaded node binaries

* Original sha: 955972b
* Authored by ppisljar <[email protected]> on 2016-07-11T19:17:08Z

**Commit 2:**
only skipping download if --skip-download cli argument is present

* Original sha: 325e172
* Authored by ppisljar <[email protected]> on 2016-09-07T10:54:23Z

**Commit 3:**
updating log messages based on epixas comments

* Original sha: 20b5c4d
* Authored by ppisljar <[email protected]> on 2016-09-23T06:24:55Z

**Commit 4:**
updating based on courts review

* Original sha: 78c124c
* Authored by ppisljar <[email protected]> on 2016-10-29T14:44:43Z
ppisljar pushed a commit that referenced this issue Oct 29, 2016
Backports PR #7746

**Commit 1:**
fix #7136 - check SHA of downloaded node binaries

* Original sha: 955972b
* Authored by ppisljar <[email protected]> on 2016-07-11T19:17:08Z

**Commit 2:**
only skipping download if --skip-download cli argument is present

* Original sha: 325e172
* Authored by ppisljar <[email protected]> on 2016-09-07T10:54:23Z

**Commit 3:**
updating log messages based on epixas comments

* Original sha: 20b5c4d
* Authored by ppisljar <[email protected]> on 2016-09-23T06:24:55Z

**Commit 4:**
updating based on courts review

* Original sha: 78c124c
* Authored by ppisljar <[email protected]> on 2016-10-29T14:44:43Z
epixa pushed a commit that referenced this issue Oct 29, 2016
Backports PR #7746

**Commit 1:**
fix #7136 - check SHA of downloaded node binaries

* Original sha: 955972b
* Authored by ppisljar <[email protected]> on 2016-07-11T19:17:08Z

**Commit 2:**
only skipping download if --skip-download cli argument is present

* Original sha: 325e172
* Authored by ppisljar <[email protected]> on 2016-09-07T10:54:23Z

**Commit 3:**
updating log messages based on epixas comments

* Original sha: 20b5c4d
* Authored by ppisljar <[email protected]> on 2016-09-23T06:24:55Z

**Commit 4:**
updating based on courts review

* Original sha: 78c124c
* Authored by ppisljar <[email protected]> on 2016-10-29T14:44:43Z
nreese pushed a commit to nreese/kibana that referenced this issue Nov 10, 2016
* fix elastic#7136 - check SHA of downloaded node binaries
* skips download if --skip-node-download cli argument is present
airow pushed a commit to airow/kibana that referenced this issue Feb 16, 2017
Backports PR elastic#7746

**Commit 1:**
fix elastic#7136 - check SHA of downloaded node binaries

* Original sha: 955972b
* Authored by ppisljar <[email protected]> on 2016-07-11T19:17:08Z

**Commit 2:**
only skipping download if --skip-download cli argument is present

* Original sha: 325e172
* Authored by ppisljar <[email protected]> on 2016-09-07T10:54:23Z

**Commit 3:**
updating log messages based on epixas comments

* Original sha: 20b5c4d
* Authored by ppisljar <[email protected]> on 2016-09-23T06:24:55Z

**Commit 4:**
updating based on courts review

* Original sha: 78c124c
* Authored by ppisljar <[email protected]> on 2016-10-29T14:44:43Z

Former-commit-id: 8e22a9e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience good first issue low hanging fruit PR sent Team:Operations Team label for Operations Team
Projects
None yet
Development

No branches or pull requests

6 participants