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

Add checksum verification of bash script #282

Merged
merged 8 commits into from Apr 16, 2021

Conversation

thomasrockhu
Copy link
Contributor

@thomasrockhu thomasrockhu commented Apr 15, 2021

Adds checksum validation for the underlying bash script
fixes #281

@thomasrockhu thomasrockhu changed the title Test version pulling Add checksum verification of bash script Apr 15, 2021
@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #282 (864620a) into master (9b0b9bb) will increase coverage by 1.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
+ Coverage   96.42%   97.66%   +1.23%     
==========================================
  Files           3        4       +1     
  Lines         140      171      +31     
  Branches       43       49       +6     
==========================================
+ Hits          135      167      +32     
+ Misses          5        4       -1     
Flag Coverage Δ
demo 87.50% <ø> (ø)
script 98.70% <100.00%> (+1.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/validate.ts 100.00% <100.00%> (ø)
src/buildExec.ts 98.38% <0.00%> (+0.80%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b0b9bb...864620a. Read the comment docs.

@tovbinm
Copy link

tovbinm commented Apr 16, 2021

Any ETA on adding this?

@thomasrockhu
Copy link
Contributor Author

@tovbinm working on it as quickly as I can, I hope to have it finished by tomorrow EOD

@tovbinm
Copy link

tovbinm commented Apr 16, 2021

Thank you!

@thomasrockhu thomasrockhu marked this pull request as ready for review April 16, 2021 02:27
@thomasrockhu thomasrockhu requested a review from a team April 16, 2021 02:28
src/validate.ts Outdated Show resolved Hide resolved
return false;
}

for (const i of [1, 256, 512]) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of checking all the checksums? Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was intentional, although it is unlikely that one would match and another wouldn't, I'd prefer to be thorough

@thomasrockhu thomasrockhu merged commit ce1ffb8 into master Apr 16, 2021
@thomasrockhu thomasrockhu deleted the add-checksum-verification branch April 16, 2021 15:21
@haampie
Copy link

haampie commented Apr 20, 2021

Wait... you're downloading a checksum? That's not how checksum validation works

@tovbinm
Copy link

tovbinm commented Apr 20, 2021

Computed checksum is compared against the published one.

@Artoria2e5
Copy link

Artoria2e5 commented Apr 20, 2021

The big question is… Who checks the checksums? If it's in the same repo as the downloaded file as it appears to he now, anyone who has the ability to change the script also has the ability to modify the checksum to match.

You need a better source of verification than this one. The traditional way is to GPG-sign the checksum, but I'd wager you can somehow query GitHub about whether the commit or tag is "verified" (i.e. GPG signed or done on GitHub web). {Yep, there is one. Ctrl-F https://docs.github.com/en/rest/reference/git for the verified field.}

Now is getting verified enough? Verification asserts that the commit added by a particular user is signed by them, and in theory an attacker can create a throwaway account for that. This should be enough, as having some random GitHub user make commits imply way bigger problems…

@tovbinm
Copy link

tovbinm commented Apr 20, 2021

The validation is done in the GHA code each time one would run a build. It compares the hashes computed for a hosted bash script (codecov.io/bash) and the published values on GitHub.

@Artoria2e5
Copy link

Artoria2e5 commented Apr 20, 2021

Hmm, turns out the hosted version doesn't come from GitHub, but some Google Cloud stuff. Cool… I guess? At least it's in two places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checksum should be run on bash uploader script before execution
6 participants