-
Notifications
You must be signed in to change notification settings - Fork 89
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 support for validating GitHub Release Asset checksums #34
Changes from 3 commits
9205b82
fd6fdb2
343ea70
3bb9834
2d3fcfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
package main | ||
|
||
import ( | ||
"fmt" | ||
"crypto/sha256" | ||
"crypto/sha512" | ||
"os" | ||
"io" | ||
"hash" | ||
"encoding/hex" | ||
) | ||
|
||
func verifyChecksumOfReleaseAsset(assetPath, checksum, algorithm string) *FetchError { | ||
computedChecksum, err := computeChecksum(assetPath, algorithm) | ||
if err != nil { | ||
return newError(ERROR_WHILE_COMPUTING_CHECKSUM, err.Error()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's generally nicer when writing tests and debugging to return a different type for each, well, type of error. All you do is define a struct (or even a type alias) and add an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hear you. This was an early Golang tool I wrote and I didn't want to refactor all the errors throughout the code so I stuck with the existing idiom. |
||
} | ||
if computedChecksum != checksum { | ||
return newError(CHECKSUM_DOES_NOT_MATCH, fmt.Sprintf("Expected to receive checksum value %s, but instead got %s for Release Asset at %s", computedChecksum, checksum, assetPath)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to provide a bit more info about what this could mean in this error message. E.g, "This means that either you are using the wrong checksum value in your call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good idea. I'll update now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
fmt.Printf("Checksum matches!") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not use any sort of logging library with fetch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was surprised about that as well. Again, I didn't want to invest time in updating those kinds of things so I stuck with the existing idiom. |
||
|
||
return nil | ||
} | ||
|
||
func computeChecksum(filePath string, algorithm string) (string, error) { | ||
var checksum string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a useless mutable value to have. I'd just return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I sometimes like to declare the return var at the top, but your suggestion is probably a little cleaner. updated. |
||
|
||
file, err := os.Open(filePath) | ||
if err != nil { | ||
return checksum, err | ||
} | ||
defer file.Close() | ||
|
||
switch algorithm { | ||
case "sha256": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this case statement is identical in all but the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good suggestion. Updated. |
||
fmt.Printf("Computing checksum of release asset using SHA256\n") | ||
hasher := sha256.New() | ||
if _, err := io.Copy(hasher, file); err != nil { | ||
return checksum, err | ||
} | ||
|
||
checksum = hasherToString(hasher) | ||
case "sha512": | ||
fmt.Printf("Computing checksum of release asset using SHA512\n") | ||
hasher := sha512.New() | ||
if _, err := io.Copy(hasher, file); err != nil { | ||
return checksum, err | ||
} | ||
|
||
checksum = hasherToString(hasher) | ||
default: | ||
return checksum, fmt.Errorf("The checksum algorithm \"%s\" is not supported", algorithm) | ||
} | ||
|
||
return checksum, nil | ||
} | ||
|
||
// Convert a hasher instance (the common interface used by all Golang hashing functions) to the string value of that hasher | ||
func hasherToString(hasher hash.Hash) string { | ||
return hex.EncodeToString(hasher.Sum(nil)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
package main | ||
|
||
import ( | ||
"testing" | ||
"github.com/stretchr/testify/assert" | ||
"io/ioutil" | ||
) | ||
|
||
const SAMPLE_RELEASE_ASSET_GITHUB_REPO_URL ="https://github.com/gruntwork-io/health-checker" | ||
const SAMPLE_RELEASE_ASSET_VERSION="v0.0.2" | ||
const SAMPLE_RELEASE_ASSET_NAME="health-checker_linux_amd64" | ||
|
||
// Checksums can be computed by running "shasum -a [256|512] /path/to/file" on any UNIX system | ||
const SAMPLE_RELEASE_ASSET_CHECKSUM_SHA256="4314590d802760c29a532e2ef22689d4656d184b3daa63f96bc8b8f76f5d22f0" | ||
const SAMPLE_RELEASE_ASSET_CHECKSUM_SHA512="28d9e487c1001e3c28d915c9edd3ed37632f10b923bd94d4d9ac6d28c0af659abbe2456da167763d51def2182fef01c3f73c67edf527d4ed1389a28ba10db332" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do you users get these checksums from? If it's from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. When I went to download a third-party health checker for Kafka, I realized how risky (and yet common!) including such a library is. If an attacker wanted to exploit a library that many people use, one way is to find a way into someone's GitHub account (leaked GitHub token?), leave the commits unaltered and update only the release asset (where in most cases no checksum at all is published). In this case, I'm making the assumption that at the time you download the code, you have an opportunity to look around the repo and conclude for yourself that it appears not to be compromised. There's only so much you can do here, of course, but at least if in the future when you've long forgotten about this library, something malicious happens, you'll be notified. In the end, I suppose it's just more defense in depth. |
||
|
||
func TestVerifyReleaseAsset(t *testing.T) { | ||
tmpDir := mkTempDir(t) | ||
|
||
githubRepo, err := ParseUrlIntoGitHubRepo(SAMPLE_RELEASE_ASSET_GITHUB_REPO_URL, "") | ||
if err != nil { | ||
t.Fatalf("Failed to parse sample release asset GitHub URL into Fetch GitHubRepo struct: %s", err) | ||
} | ||
|
||
assetPath, fetchErr := downloadReleaseAsset(SAMPLE_RELEASE_ASSET_NAME, tmpDir, githubRepo, SAMPLE_RELEASE_ASSET_VERSION) | ||
if fetchErr != nil { | ||
t.Fatalf("Failed to download release asset: %s", fetchErr) | ||
} | ||
|
||
checksumSha256, fetchErr := computeChecksum(assetPath, "sha256") | ||
if fetchErr != nil { | ||
t.Fatalf("Failed to compute file checksum: %s", fetchErr) | ||
} | ||
|
||
checksumSha512, fetchErr := computeChecksum(assetPath, "sha512") | ||
if fetchErr != nil { | ||
t.Fatalf("Failed to compute file checksum: %s", fetchErr) | ||
} | ||
|
||
assert.Equal(t, SAMPLE_RELEASE_ASSET_CHECKSUM_SHA256, checksumSha256, "SHA256 checksum of sample asset failed to match.") | ||
assert.Equal(t, SAMPLE_RELEASE_ASSET_CHECKSUM_SHA512, checksumSha512, "SHA512 checksum of sample asset failed to match.") | ||
} | ||
|
||
func mkTempDir(t *testing.T) string { | ||
tmpDir, err := ioutil.TempDir("", "") | ||
if err != nil { | ||
t.Fatalf("Failed to create temp directory: %s", err) | ||
} | ||
|
||
return tmpDir | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems equally important to have checksums for modules from the
--modules
param. Probably the simplest option is to publish the commit ID (sha1) and compare that to the commit ID of the repo we just downloaded viagit clone
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the
--modules
pattern ofgruntwork-install
? Well, can't you just use Fetch to download the exact commit you want to achieve what you're suggesting?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
--modules
. We typically use tags for specifying the version we want. Should our recommendation be that users specify both the tag and the commit ID?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, keep in mind that fetch allows you to use tag constrain expressions, but I see where you're going with this. Yes, I think we should permit using both a
--tag
and a--commit-id
param at the same time. Fetch can then error out if the two don't match. I think I'll file a GitHub issue for that for now, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#35