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

An updated Asset SHA can fail to display #1467

Closed
portertech opened this issue May 7, 2018 · 5 comments · Fixed by #1547
Closed

An updated Asset SHA can fail to display #1467

portertech opened this issue May 7, 2018 · 5 comments · Fixed by #1547
Assignees
Labels

Comments

@portertech
Copy link
Contributor

screen shot 2018-05-07 at 3 48 54 pm

@portertech portertech added the bug label May 7, 2018
@portertech
Copy link
Contributor Author

Bug update, the value is there on a following asset update:

screen shot 2018-05-07 at 3 51 04 pm

@portertech portertech changed the title Updating an Asset SHA fails An updated Asset SHA can fail to display May 7, 2018
@grepory
Copy link
Contributor

grepory commented May 8, 2018

Some kind of input validation is going to be required to make sure that we're not storing invalid checksums / checksums aren't getting set to nil.

@nikkictl
Copy link

The asset sha is only failing to display because the updated sha nope is < 7 characters.

if len(asset.Sha512) >= 7 {
return string(asset.Sha512[0:7])
}
return ""

A quick fix would be something like this:

    length := len(asset.Sha512)

    if length >= 7 {
      return string(asset.Sha512[0:7])
    }
    return string(asset.Sha512[0:length])

But Greg (AFK so not @-ing him here) mentioned that we shouldn't be storing invalid checksums (we already validate it cannot be empty). @portertech do you think this proposed solution is sufficient or would you rather we have a more strict checksum validation at the creation of an asset?

@palourde
Copy link
Contributor

IMO we should ensure the SHA512 is 128 characters, otherwise there's no way it's valid.

@nikkictl
Copy link

nikkictl commented May 16, 2018

@palourde 👍

sensu-go (master)$ bin/sensuctl asset update sensu-influxdb-handler
? URL: https://github.com/portertech/sensu-plugins-go/releases/download/0.0.1/sensu-check-plugins.tar.gz
? SHA-512 Checksum: nope
Error: SHA-512 checksum must be at least 128 characters

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

Successfully merging a pull request may close this issue.

6 participants