feat(signature): Checksum signature verification #1670
Conversation
| @@ -616,7 +679,7 @@ main() ( | |||
| install_dir=${install_dir:-./bin} | |||
|
|
|||
| # note: never change the program flags or arguments (this must always be backwards compatible) | |||
There was a problem hiding this comment.
note: never change the program flags or arguments (this must always be backwards compatible)
A note for reviewers/testers: we need to verify that this would function when installing a previous release. Ideally we'd know the specific release that implemented this and be able to short circuit this and error when validation would silently never occur.
There was a problem hiding this comment.
(Note: currently unknown options print usage but are ignored, so old releases would download without checking anything. If you want older releases to fail, we'll need to compare the version given here with the first release that supports this as you suggest)
There was a problem hiding this comment.
This is an awesome add! I need to think through and run some testing to ensure installing previous releases is ok https://github.com/anchore/grype/pull/1670/files#r1467020629
|
@wagoodman Any update? As you mentioned, definitely need a check to test which version this feature was rolled out and show error when |
install.sh
Outdated
| return 1 | ||
| fi | ||
|
|
||
| checksum_sig_file_path=$(download_github_release_checksums_sig "${download_url}" "${name}" "${version}" "${destination}") |
There was a problem hiding this comment.
Not a problem per se, but cosign can be passed an url directly, is there any benefit to downloading the cert/signatures?
e.g. this worked (for syft but it's similar)
cosign verify-blob syft_1.4.1_checksums.txt \
--certificate https://github.com/anchore/syft/releases/download/v1.4.1/syft_1.4.1_checksums.txt.pem \
--signature https://github.com/anchore/syft/releases/download/v1.4.1/syft_1.4.1_checksums.txt.sig \
--certificate-oidc-issuer "https://token.actions.githubusercontent.com" \
--certificate-identity 'https://github.com/anchore/syft/.github/workflows/release.yaml@refs/heads/main'
There was a problem hiding this comment.
I'll push up some changes that use the URL over curling down the material ourself, which would reduce the attackable surface area here (there isn't a cert/signature ever available on disk to manipulate).
install.sh
Outdated
| ${COSIGN_BINARY} verify-blob "$1" \ | ||
| --certificate "$2" \ | ||
| --signature "$3" \ | ||
| --certificate-identity-regexp "https://github\.com/${OWNER}/${REPO}/\.github/workflows/.+" \ |
There was a problem hiding this comment.
We probably should be more strict than the regex for this, otherwise I assume I could open a PR that adds a new workflow that'll sign something bad™ -- that'll leave traces but it'll still be usable for old releases.
Since the path currently doesn't change, we can just use it as is:
--certificate-identity "https://github.com/${OWNER}/${REPO}/.github/workflows/release.yaml@refs/heads/main"
Ideally the build script should be updated to build releases by tag (refs/tags/v1.2.3) instead of branch, at which point we'll be able to say @refs/tags/$version
There was a problem hiding this comment.
We can't quite switch to @refs/tags/$version yet since we tag from within the release workflow itself, which means that the OIDC token will have the identity containing refs/heads/main specifically and not the tag (since it wouldn't have existed yet).
Why do we tag from within the workflow? We used to not do this, however, the act of tagging in a go repo is essentially no different from releasing from the go proxies point of view, because if you ever need to delete the tag and retag to a different commit then you'll start getting h1 mismatches when folks go get your module. There is a bit of a race condition against the tagging and the go proxy, but in the pessimistic case (when the proxy wins the race) then this is an issue.
...that all being said, I don't disagree with your point: it would be better to reference the tag and not the branch, but that can be an incremental improvement later. In the meantime I've made a change to lock it to the main branch 👍
There was a problem hiding this comment.
Thanks for the explanation on the release process!
It makes sense that we don't want to "mess up" tags and do them in the pipeline, and we want to build the artifacts first to ensure there hasn't been a mishap somewhere; I'm sure something could be figured out (can we sign with a local not-pushed tag?) but that's fine as a later improvement as you've said; locking to main branch is already good enough for me.
install.sh
Outdated
| checksum_sig_file_path=$(download_github_release_checksums_sig "${download_url}" "${name}" "${version}" "${destination}") | ||
| log_info "downloaded checksums signature file: ${checksum_sig_file_path}" | ||
|
|
||
| checksums_cert_file_path=$(download_github_release_checksums_cert "${download_url}" "${name}" "${version}" "${destination}") |
There was a problem hiding this comment.
That's more of a cosign question than a question about this script, but if we download the signing certificate what prevents an attacker to provide another checksums file/certificate/signature set?
I'd assume it's the github oidc feature, because the authority cosign has built in wouldn't issue that tokens.actions.githubusercontent.com issuer intermediate certificate to anyone else, and github wouldn't give this certificate identity to another workflow, but given the tool didn't make any network request I don't like the fact that there doesn't seem to be a CRL, so if a bug allows malicious users to abuse the github token they'll be able to provide their own set of files? Meaning I'd be much more comfortable pinning a fingerprint we could just update if it leaks...
But now I'm looking it looks like each release generates a different certificate every time, so that wouldn't work. I guess I'll have to learn to trust these tools...
| @@ -616,7 +679,7 @@ main() ( | |||
| install_dir=${install_dir:-./bin} | |||
|
|
|||
| # note: never change the program flags or arguments (this must always be backwards compatible) | |||
There was a problem hiding this comment.
(Note: currently unknown options print usage but are ignored, so old releases would download without checking anything. If you want older releases to fail, we'll need to compare the version given here with the first release that supports this as you suggest)
|
@hibare I had some time to help with adjustments here -- I originally started adding detailed comments, but instead felt that pairing on getting this over the finish line would go further/faster. Here's the adjustments I've made:
|
install.sh
Outdated
| # compare two semver strings. Returns 0 if version1 >= version2, 1 otherwise. | ||
| # Note: pre-release (-) and metadata (+) are not supported. | ||
| compare_semver() { | ||
| # remove leading 'v' if present | ||
| version1=$(echo "$1" | sed 's/^v//') | ||
| version2=$(echo "$2" | sed 's/^v//') | ||
|
|
||
| IFS=. read -r major1 minor1 patch1 <<EOF | ||
| $version1 | ||
| EOF | ||
| IFS=. read -r major2 minor2 patch2 <<EOF | ||
| $version2 | ||
| EOF | ||
|
|
||
| if [ "$major1" -gt "$major2" ]; then | ||
| return 0 | ||
| elif [ "$major1" -lt "$major2" ]; then | ||
| return 1 | ||
| fi | ||
|
|
||
| if [ "$minor1" -gt "$minor2" ]; then | ||
| return 0 | ||
| elif [ "$minor1" -lt "$minor2" ]; then | ||
| return 1 | ||
| fi | ||
|
|
||
| if [ "$patch1" -gt "$patch2" ]; then | ||
| return 0 | ||
| elif [ "$patch1" -lt "$patch2" ]; then | ||
| return 1 | ||
| fi | ||
|
|
||
| # versions are equal | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
(nitpick) coreutils sort and busybox sort at least support sort -V (version sort) and -c (check), so that can be used to check in a quite shorter function:
| # compare two semver strings. Returns 0 if version1 >= version2, 1 otherwise. | |
| # Note: pre-release (-) and metadata (+) are not supported. | |
| compare_semver() { | |
| # remove leading 'v' if present | |
| version1=$(echo "$1" | sed 's/^v//') | |
| version2=$(echo "$2" | sed 's/^v//') | |
| IFS=. read -r major1 minor1 patch1 <<EOF | |
| $version1 | |
| EOF | |
| IFS=. read -r major2 minor2 patch2 <<EOF | |
| $version2 | |
| EOF | |
| if [ "$major1" -gt "$major2" ]; then | |
| return 0 | |
| elif [ "$major1" -lt "$major2" ]; then | |
| return 1 | |
| fi | |
| if [ "$minor1" -gt "$minor2" ]; then | |
| return 0 | |
| elif [ "$minor1" -lt "$minor2" ]; then | |
| return 1 | |
| fi | |
| if [ "$patch1" -gt "$patch2" ]; then | |
| return 0 | |
| elif [ "$patch1" -lt "$patch2" ]; then | |
| return 1 | |
| fi | |
| # versions are equal | |
| return 0 | |
| } | |
| # compare two semver strings. Returns 0 if version1 >= version2, 1 otherwise. | |
| # Note: pre-release (-) and metadata (+) are not supported. | |
| compare_semver() { | |
| # remove leading 'v' if present | |
| local version1=${1#v} | |
| local version2=${2#v} | |
| # this checks $2 <= $1, so returns 0 if first argument <= second | |
| printf "%s\n" "$version2" "$version1" | sort -Vc 2>/dev/null | |
| } |
quick check done with (and now I noticed it, it also passes the test you added):
compare_semver v0.72.0 v0.72.0 || echo bad1
compare_semver v0.72.0 v0.73.0 && echo bad2
compare_semver v0.73.0 v0.72.0 || echo bad3
compare_semver 0.72.0 v0.72.0 || echo bad4
compare_semver 0.172.0 0.72.0 || echo bad5
Unfortunately it seems to think that 0.1-rc is higher than 0.1 which isn't semver-compliant (rc is before the release), but we probably don't really care here so I just left the comment.
(ah, this script doesn't seem to use any 'local', I don't know any shell which doesn't support the keyword but it's true that it's not posix so feel free to drop these. ${1#v} is posix-sh compliant.)
There was a problem hiding this comment.
clever! I like this implementation better. I'll poke at it with a few more tests to make certain it works (I've been caught in situations where lexically sorting vs parse-and-sort appeared to be the same but weren't).
There was a problem hiding this comment.
so close! it seems like there is a quirk with older versions of alpine... probably a bug on their part? but I haven't dug:
❯ docker run --rm -it alpine:3.6 ash
/ # compare_semver() {
> # remove leading 'v' if present
> local version1=${1#v}
> local version2=${2#v}
>
> # this checks $2 <= $1, so returns 0 if first argument <= second
> printf "%s\n" "$version2" "$version1" | sort -Vc 2>/dev/null
> }
/ # compare_semver v0.72.0 v0.72.0 || echo bad1
bad1
But seems to work fine with later versions of alpine:
❯ docker run --rm -it alpine:latest ash
/ # compare_semver() {
> # remove leading 'v' if present
> local version1=${1#v}
> local version2=${2#v}
>
> # this checks $2 <= $1, so returns 0 if first argument <= second
> printf "%s\n" "$version2" "$version1" | sort -Vc 2>/dev/null
> }
/ # compare_semver v0.72.0 v0.72.0 || echo bad1
/ #
I'll keep the original (verbose) implementation, but it isn't immediately clear why your approach doesn't work in these cases.
There was a problem hiding this comment.
alpine 3.6 sure is old! That version of busybox sort didn't have -V yet:
# sort -V
sort: unrecognized option: V
Checking other old alpines it's available since 3.10.
alpine 3.9 has been EOL since 2020-11-01 though so I'm not sure how much you need to care, but there is no problem with the pure shell version as far as I'm aware so let's keep it this way :)
| # don't continue if we couldn't install the asset | ||
| if [ "$?" != "0" ]; then | ||
| if ! download_and_install_asset "${download_url}" "${download_dir}" "${install_dir}" "${PROJECT_NAME}" "${os}" "${arch}" "${version}" "${format}" "${binary}"; then |
There was a problem hiding this comment.
(nitpick, unrelated to this patch) it's generally bad practice to have too many arguments, aside of PROJECT_NAME/name all the variables are the same so I'd just skip passing them around.
Honestly just noticed because we did bother splitting the cosign command on multiple lines and not this, feel free to ignore
There was a problem hiding this comment.
it's generally bad practice to have too many arguments
💯 entirely agreed, this is where my bash-foo posix-compliant-shell-foo drops off a cliff. When a function needs a lot of input I feel like I have to resort to lots of args, over use of globals, or other weird things. Since I've got my hands in this I'll see if there is any opportunity to make this section better 🤔
Signed-off-by: Shubham Hibare <shubham@hibare.in> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Shubham Hibare <shubham@hibare.in> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Shubham Hibare <shubham@hibare.in> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Co-authored-by: Dominique Martinet <asmadeus@codewreck.org> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Co-authored-by: Dominique Martinet <asmadeus@codewreck.org> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
|
Thanks @hibare and @martinetd for the contribution and reviews! |
|
Thanks for the work! |
* feat(signature): Checksum signature verification Signed-off-by: Shubham Hibare <shubham@hibare.in> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * Update message Signed-off-by: Shubham Hibare <shubham@hibare.in> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * address comments Signed-off-by: Shubham Hibare <shubham@hibare.in> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * consider -v flag across supported releases Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * add tests for install.sh signature verification Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * check that release is run from main Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * summarize install.sh flags and recommendations Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * remove regex use on cosign verify-blob Co-authored-by: Dominique Martinet <asmadeus@codewreck.org> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * simplify the compare_semver install function Co-authored-by: Dominique Martinet <asmadeus@codewreck.org> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * add more tests to compare_semver Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * nit copy change for install help Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * keep original compare_semver implementation Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * update copy to include default install path Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> --------- Signed-off-by: Shubham Hibare <shubham@hibare.in> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> Co-authored-by: Alex Goodman <wagoodman@users.noreply.github.com> Co-authored-by: Dominique Martinet <asmadeus@codewreck.org>
Closes #1627
This PR adds checksum.txt file signature verification before downloading and installing actual binary. This is an optional opt-in feature using command line flag
-vto installation script.Signature verification process depends on 3rd party cosign binary. If the binary is not found, the user is prompted to install the binary. Cosign binary installation is not part of this script.
The overall process with signature verification looks like this:
-vcommand line flag to installation script.Successful signature verification:
Signature verification failure:
Cosign binary not found: