fix(security): verify SHA-256 checksum on OpenShell binary download#697
Merged
fix(security): verify SHA-256 checksum on OpenShell binary download#697
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe install script for OpenShell now includes SHA-256 checksum verification. It downloads a checksums file, validates the downloaded archive against it, and only proceeds with extraction upon successful verification. A regression test ensures these safety checks remain present in the script. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/install-openshell.sh (2)
82-98: Checksum verification correctly implemented with proper cleanup.This script has the EXIT trap at line 80, ensuring
$tmpdiris cleaned up regardless of success or failure. The checksum verification logic is consistent withscripts/install.sh.Same minor note about grep precision applies here:
♻️ Optional: Use fixed-string grep for precision
- (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ + (cd "$tmpdir" && grep -F "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-openshell.sh` around lines 82 - 98, The checksum verification uses grep "$ASSET" "$CHECKSUM_FILE" which can mis-match substrings; update the verification command to use a fixed-string or exact-match grep (e.g., use grep -F or grep -x) so CHECKSUM_FILE is searched precisely for the ASSET entry before piping to shasum -a 256 -c -; modify the line that runs (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) to reference the chosen grep option and ensure variables GH_TOKEN, ASSET, CHECKSUM_FILE and tmpdir remain unchanged.
96-97: Consider handling missing checksum entry explicitly.If
grep "$ASSET"finds no match (asset not listed in checksum file), the pipeline will fail with "SHA-256 checksum verification failed." This is technically correct, but the error message could be misleading—the failure isn't a mismatch but a missing entry.♻️ Optional: More informative error handling
info "Verifying SHA-256 checksum..." -(cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ - || fail "SHA-256 checksum verification failed for $ASSET" +(cd "$tmpdir" && { + checksum_line=$(grep -F "$ASSET" "$CHECKSUM_FILE") \ + || fail "Asset $ASSET not found in $CHECKSUM_FILE" + echo "$checksum_line" | shasum -a 256 -c - +}) || fail "SHA-256 checksum verification failed for $ASSET"This is optional since the current behavior is safe (fails closed), just less descriptive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-openshell.sh` around lines 96 - 97, The failure message is ambiguous when the checksum file lacks an entry for ASSET; before piping into shasum, test for a matching line in CHECKSUM_FILE (e.g., use grep -q -- "$ASSET" "$CHECKSUM_FILE" or capture the grep output into a variable), and if no match call fail with a clear message like "No checksum entry for $ASSET in $CHECKSUM_FILE"; otherwise feed the matching line to shasum -a 256 -c - and keep the existing failure path for actual checksum mismatches.scripts/install.sh (1)
339-356: Good security improvement with checksum verification.The implementation correctly downloads and verifies the SHA-256 checksum before extraction. A few observations:
Missing tmpdir cleanup on failure: Unlike
install-openshell.shwhich hastrap 'rm -rf "$tmpdir"' EXIT(line 80), this script does not set up a trap. If checksum verification fails,$tmpdiris left behind.Grep pattern could be more precise:
grep "$ASSET"could match multiple lines if a filename is a substring of another (e.g.,openshell-x86_64matching bothopenshell-x86_64-apple-darwin.tar.gzand a hypotheticalopenshell-x86_64-v2-apple-darwin.tar.gz). Consider anchoring the pattern.♻️ Suggested improvements
tmpdir="$(mktemp -d)" + trap 'rm -rf "$tmpdir"' EXIT CHECKSUM_FILE="openshell-checksums-sha256.txt"For more precise grep matching (optional):
- (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ + (cd "$tmpdir" && grep -F "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \Using
-Ftreats the pattern as a fixed string rather than a regex, which is safer and slightly faster.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 339 - 356, The script currently downloads checksums into $tmpdir but doesn't register a cleanup trap (leaving tmpdir if verification fails) and uses grep "$ASSET" which can match substrings; add a trap like the one in install-openshell.sh (trap 'rm -rf "$tmpdir"' EXIT) immediately after creating tmpdir to ensure removal on exit/failure, and make the verification grep precise by using fixed/anchored matching (e.g., use grep -F or anchor the pattern to the exact filename when reading CHECKSUM_FILE) in the (cd "$tmpdir" && grep ... "$CHECKSUM_FILE" | shasum -a 256 -c -) step so only the exact ASSET entry is checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install-openshell.sh`:
- Around line 82-98: The checksum verification uses grep "$ASSET"
"$CHECKSUM_FILE" which can mis-match substrings; update the verification command
to use a fixed-string or exact-match grep (e.g., use grep -F or grep -x) so
CHECKSUM_FILE is searched precisely for the ASSET entry before piping to shasum
-a 256 -c -; modify the line that runs (cd "$tmpdir" && grep "$ASSET"
"$CHECKSUM_FILE" | shasum -a 256 -c -) to reference the chosen grep option and
ensure variables GH_TOKEN, ASSET, CHECKSUM_FILE and tmpdir remain unchanged.
- Around line 96-97: The failure message is ambiguous when the checksum file
lacks an entry for ASSET; before piping into shasum, test for a matching line in
CHECKSUM_FILE (e.g., use grep -q -- "$ASSET" "$CHECKSUM_FILE" or capture the
grep output into a variable), and if no match call fail with a clear message
like "No checksum entry for $ASSET in $CHECKSUM_FILE"; otherwise feed the
matching line to shasum -a 256 -c - and keep the existing failure path for
actual checksum mismatches.
In `@scripts/install.sh`:
- Around line 339-356: The script currently downloads checksums into $tmpdir but
doesn't register a cleanup trap (leaving tmpdir if verification fails) and uses
grep "$ASSET" which can match substrings; add a trap like the one in
install-openshell.sh (trap 'rm -rf "$tmpdir"' EXIT) immediately after creating
tmpdir to ensure removal on exit/failure, and make the verification grep precise
by using fixed/anchored matching (e.g., use grep -F or anchor the pattern to the
exact filename when reading CHECKSUM_FILE) in the (cd "$tmpdir" && grep ...
"$CHECKSUM_FILE" | shasum -a 256 -c -) step so only the exact ASSET entry is
checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aed8a69f-7e86-4cd5-94b6-bd88862a8fe3
📒 Files selected for processing (3)
scripts/install-openshell.shscripts/install.shtest/runner.test.js
cv
force-pushed
the
fix/openshell-checksum-verify
branch
from
4233787 to
223e46e
Compare
wscurran
added
enhancement
cv
requested changes
Mar 23, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The checksum verification works for detecting transport corruption (truncated downloads, bit flips). However, both the binary and openshell-checksums-sha256.txt are fetched from the same GitHub release — an attacker who can compromise the release artifacts can replace both. This doesn't protect against the supply chain threat that fix(security) implies.
Filed upstream: NVIDIA/OpenShell#542 requests artifact attestations, which would provide cryptographic provenance verification.
Two items before merge:
install.shis missing atrap 'rm -rf "$tmpdir"' EXIT—install-openshell.shhas one (line 80), butinstall.shdoes not. On checksum failure, the downloaded binary persists on disk.- Consider
grep -Finstead ofgrepfor the checksum lookup —$ASSETcontains dots and hyphens that are regex metacharacters.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.sh (1)
358-360: Use exact field matching for checksum file lookup.At line 359,
grep "$ASSET"matches by regex/substring. For more robust checksum verification, match the exact filename field and verify exactly one entry exists.Proposed patch
info "Verifying SHA-256 checksum..." - (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ + (cd "$tmpdir" && \ + awk -v asset="$ASSET" ' + $2 == asset { print; count++ } + END { exit(count == 1 ? 0 : 1) } + ' "$CHECKSUM_FILE" | shasum -a 256 -c -) \ || fail "SHA-256 checksum verification failed for $ASSET"This ensures the checksum file lookup matches the exact filename field rather than substring matching, preventing potential ambiguity if similarly named assets are added in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 358 - 360, The checksum lookup currently uses grep "$ASSET" which can match substrings; instead, in the block that runs in (cd "$tmpdir" && ... | shasum -a 256 -c -) use a strict field match against the filename (ASSET) in CHECKSUM_FILE and verify exactly one matching line is returned before piping to shasum. Concretely, replace the grep step with a precise extractor (e.g., awk or grep -F with field anchoring) that matches the filename field equal to the ASSET variable, count matches and fail if not exactly one, then pass that single line to shasum -a 256 -c -. Keep references to tmpdir, ASSET and CHECKSUM_FILE when locating code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install.sh`:
- Around line 358-360: The checksum lookup currently uses grep "$ASSET" which
can match substrings; instead, in the block that runs in (cd "$tmpdir" && ... |
shasum -a 256 -c -) use a strict field match against the filename (ASSET) in
CHECKSUM_FILE and verify exactly one matching line is returned before piping to
shasum. Concretely, replace the grep step with a precise extractor (e.g., awk or
grep -F with field anchoring) that matches the filename field equal to the ASSET
variable, count matches and fail if not exactly one, then pass that single line
to shasum -a 256 -c -. Keep references to tmpdir, ASSET and CHECKSUM_FILE when
locating code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d2bfc6e-5547-4eb2-a3d4-80ccfa89a488
📒 Files selected for processing (3)
scripts/install-openshell.shscripts/install.shtest/runner.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/install-openshell.sh
- test/runner.test.js
3 tasks
fdzdev
approved these changes
Mar 25, 2026
Contributor
fdzdev
left a comment
fdzdev
left a comment
There was a problem hiding this comment.
Looks good. Checksum is verified before extraction in both the gh and curl
paths, and set -e ensures a missing checksum file fails the script early
(fail-closed). Regression tests cover both install scripts.
One minor note: if NVIDIA/OpenShell ever ships a release without
openshell-checksums-sha256.txt, the install will hard-fail. That's the
right behavior, but worth a note in the release checklist upstream.
…m-verify # Conflicts: # scripts/install.sh
- Use grep -F for fixed-string matching in install-openshell.sh (ASSET contains dots/hyphens that are regex metacharacters) - Remove stale install.sh checksum test (scripts/install.sh is now a thin legacy wrapper that delegates to root install.sh)
Contributor
Author
|
Both items addressed:
Also removed the stale regression test that checked |
cv
approved these changes
Mar 30, 2026
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
…697) ## Summary `scripts/install-openshell.sh` downloads the OpenShell binary via `curl`/`gh` and installs it (often as root) without any integrity verification. OpenShell releases include `openshell-checksums-sha256.txt` alongside the tarballs — verified present in v0.0.10 through v0.0.13. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> ## Changes - Download `openshell-checksums-sha256.txt` alongside the tarball (both `gh` and `curl` paths) - Verify via `shasum -a 256 -c` before extraction - Fail with a clear error message if verification fails - Use `grep -F` for fixed-string matching (review feedback from @cv) ## Type of Change - [x] Code change for a new feature, bug fix, or refactor. ## Testing - [x] `npx prek run --all-files` passes (or equivalently `make check`). - [x] `npm test` passes (649/649). - [x] Verified checksum file exists in all recent OpenShell releases (v0.0.10–v0.0.13) - [ ] Manual: `nemoclaw onboard` installs OpenShell with checksum verification - [ ] Manual: tamper with downloaded tarball → verify installation fails with clear error ## Checklist ### General - [x] I have read and followed the [contributing guide](https://github.com/NVIDIA/NemoClaw/blob/main/CONTRIBUTING.md). ### Code Changes - [x] Formatters applied — `npx prek run --all-files` auto-fixes formatting. - [x] Tests added or updated for new or changed behavior. - [x] No secrets, API keys, or credentials committed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit **Chores** * Installation script now downloads verification checksums alongside archives and validates all downloads using SHA-256 before extracting files. Installation fails safely if validation fails. **Tests** * Added regression test to verify SHA-256 checksum validation remains integrated in the installation process. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
…VIDIA#697) ## Summary `scripts/install-openshell.sh` downloads the OpenShell binary via `curl`/`gh` and installs it (often as root) without any integrity verification. OpenShell releases include `openshell-checksums-sha256.txt` alongside the tarballs — verified present in v0.0.10 through v0.0.13. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> ## Changes - Download `openshell-checksums-sha256.txt` alongside the tarball (both `gh` and `curl` paths) - Verify via `shasum -a 256 -c` before extraction - Fail with a clear error message if verification fails - Use `grep -F` for fixed-string matching (review feedback from @cv) ## Type of Change - [x] Code change for a new feature, bug fix, or refactor. ## Testing - [x] `npx prek run --all-files` passes (or equivalently `make check`). - [x] `npm test` passes (649/649). - [x] Verified checksum file exists in all recent OpenShell releases (v0.0.10–v0.0.13) - [ ] Manual: `nemoclaw onboard` installs OpenShell with checksum verification - [ ] Manual: tamper with downloaded tarball → verify installation fails with clear error ## Checklist ### General - [x] I have read and followed the [contributing guide](https://github.com/NVIDIA/NemoClaw/blob/main/CONTRIBUTING.md). ### Code Changes - [x] Formatters applied — `npx prek run --all-files` auto-fixes formatting. - [x] Tests added or updated for new or changed behavior. - [x] No secrets, API keys, or credentials committed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit **Chores** * Installation script now downloads verification checksums alongside archives and validates all downloads using SHA-256 before extracting files. Installation fails safely if validation fails. **Tests** * Added regression test to verify SHA-256 checksum validation remains integrated in the installation process. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
…VIDIA#697) ## Summary `scripts/install-openshell.sh` downloads the OpenShell binary via `curl`/`gh` and installs it (often as root) without any integrity verification. OpenShell releases include `openshell-checksums-sha256.txt` alongside the tarballs — verified present in v0.0.10 through v0.0.13. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> ## Changes - Download `openshell-checksums-sha256.txt` alongside the tarball (both `gh` and `curl` paths) - Verify via `shasum -a 256 -c` before extraction - Fail with a clear error message if verification fails - Use `grep -F` for fixed-string matching (review feedback from @cv) ## Type of Change - [x] Code change for a new feature, bug fix, or refactor. ## Testing - [x] `npx prek run --all-files` passes (or equivalently `make check`). - [x] `npm test` passes (649/649). - [x] Verified checksum file exists in all recent OpenShell releases (v0.0.10–v0.0.13) - [ ] Manual: `nemoclaw onboard` installs OpenShell with checksum verification - [ ] Manual: tamper with downloaded tarball → verify installation fails with clear error ## Checklist ### General - [x] I have read and followed the [contributing guide](https://github.com/NVIDIA/NemoClaw/blob/main/CONTRIBUTING.md). ### Code Changes - [x] Formatters applied — `npx prek run --all-files` auto-fixes formatting. - [x] Tests added or updated for new or changed behavior. - [x] No secrets, API keys, or credentials committed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit **Chores** * Installation script now downloads verification checksums alongside archives and validates all downloads using SHA-256 before extracting files. Installation fails safely if validation fails. **Tests** * Added regression test to verify SHA-256 checksum validation remains integrated in the installation process. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts/install-openshell.shdownloads the OpenShell binary viacurl/ghand installs it (often as root) without any integrity verification. OpenShell releases includeopenshell-checksums-sha256.txtalongside the tarballs — verified present in v0.0.10 through v0.0.13.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Changes
openshell-checksums-sha256.txtalongside the tarball (bothghandcurlpaths)shasum -a 256 -cbefore extractiongrep -Ffor fixed-string matching (review feedback from @cv)Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses (649/649).nemoclaw onboardinstalls OpenShell with checksum verificationChecklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting.Summary by CodeRabbit
Chores
Tests