fix(security): add checksum verification for external binary downloads#177
fix(security): add checksum verification for external binary downloads#177dumko2001 wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
wscurran
added
enhancement
|
I've reviewed the changes and see that this PR improves security by integrating checksum verification into the asset download process, which should prevent tampering with downloaded binaries during transit. |
wscurran
added
the
priority: high
|
Hey @dumko2001, checksum verification for external downloads is exactly the kind of supply-chain hardening we want. Could you rebase this one onto the latest main? We've been shipping a lot of changes and need to see how it fits with the current code. |
dumko2001
force-pushed
the
fix/h7-brev-binary-checksums
branch
from
0a04eae to
b2b763a
Compare
📝 WalkthroughWalkthroughInstall scripts now pin OpenShell CLI to v0.0.14 and Cloudflared to 2025.2.0, select architecture-specific SHA-256 values, attempt Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User / Installer Script"
participant GH as "GitHub (gh release download)"
participant HTTP as "HTTP (curl) fallback"
participant Check as "Checksum tool\n(sha256sum / shasum)"
participant Extract as "Extract & Install"
User->>GH: Attempt `gh release download` for pinned tag
alt gh download succeeds
GH-->>User: Release artifact
else gh download fails
User->>HTTP: Curl download pinned release URL
HTTP-->>User: Release artifact
end
User->>Check: Verify artifact with EXPECTED_SHA
alt checksum matches
Check-->>User: OK
User->>Extract: Extract and install binary
Extract-->>User: Installed
else checksum fails or no tool
Check-->>User: FAIL
User-->>User: Abort installation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/install.sh (2)
352-370: Consider: Skipping verification degrades security guarantees.When neither
sha256sumnorshasumis available, the script warns but continues with extraction. While this is pragmatic for edge cases, it contradicts the PR's "zero-trust security defaults" principle. Consider whether a hard failure is more appropriate, or at minimum add a prominent note in the warning.💡 Option: fail instead of warn when verification cannot be performed
else - warn "Neither sha256sum nor shasum found, skipping checksum verification" + fail "Neither sha256sum nor shasum found. Cannot verify download integrity." fiAlternatively, if graceful degradation is intended, consider documenting this explicitly in the warning:
- warn "sha256sum not found, skipping checksum verification" + warn "⚠️ SECURITY WARNING: Checksum verification skipped — neither sha256sum nor shasum available. Install one of these tools to ensure download integrity."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 352 - 370, The script currently proceeds to extract the downloaded OpenShell asset when neither sha256sum nor shasum is available (inside the checksum verification block using EXPECTED_SHA, $tmpdir/$ASSET), which weakens the zero-trust default; update the checksum branch so that if neither sha256sum nor shasum is present the script exits non‑zero (use the same warn function to log a clear, prominent error referencing the missing tools and OPENSHELL_VERSION and then exit 1) — alternatively, if you intend to allow graceful degradation, change the warn call to a more prominent notice that documents the security implication and require an explicit opt-in environment variable (e.g., SKIP_CHECKSUM=1) before proceeding to the tar xzf "$tmpdir/$ASSET" extraction.
361-368: Minor: Warning message could be more precise.At line 367, both
sha256sumandshasumare unavailable when this branch executes. The message could better reflect this.✏️ Suggested clarification
- warn "sha256sum not found, skipping checksum verification" + warn "Neither sha256sum nor shasum found, skipping checksum verification"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 361 - 368, The warning is ambiguous when neither checksum utility is present; update the warn call in the checksum verification block (the branch that currently calls warn "sha256sum not found, skipping checksum verification") to explicitly state that neither sha256sum nor shasum was found and that checksum verification for EXPECTED_SHA against $tmpdir/$ASSET is being skipped, referencing the variables EXPECTED_SHA, tmpdir and ASSET so the message is precise and actionable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 327-330: Replace the placeholder SHA-1 in EXPECTED_SHA for the
macOS x86_64 case with the actual SHA-256 checksum of the
openshell-x86_64-apple-darwin.tar.gz release asset so checksum verification uses
a 64-character SHA-256 like the other branches; update the EXPECTED_SHA value in
the x86_64 case (where ASSET="openshell-x86_64-apple-darwin.tar.gz") to the
correct SHA-256 string and remove or update the "Placeholder for macOS" comment
accordingly.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 352-370: The script currently proceeds to extract the downloaded
OpenShell asset when neither sha256sum nor shasum is available (inside the
checksum verification block using EXPECTED_SHA, $tmpdir/$ASSET), which weakens
the zero-trust default; update the checksum branch so that if neither sha256sum
nor shasum is present the script exits non‑zero (use the same warn function to
log a clear, prominent error referencing the missing tools and OPENSHELL_VERSION
and then exit 1) — alternatively, if you intend to allow graceful degradation,
change the warn call to a more prominent notice that documents the security
implication and require an explicit opt-in environment variable (e.g.,
SKIP_CHECKSUM=1) before proceeding to the tar xzf "$tmpdir/$ASSET" extraction.
- Around line 361-368: The warning is ambiguous when neither checksum utility is
present; update the warn call in the checksum verification block (the branch
that currently calls warn "sha256sum not found, skipping checksum verification")
to explicitly state that neither sha256sum nor shasum was found and that
checksum verification for EXPECTED_SHA against $tmpdir/$ASSET is being skipped,
referencing the variables EXPECTED_SHA, tmpdir and ASSET so the message is
precise and actionable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82857277-4932-496d-ae3b-781a6e165843
📒 Files selected for processing (2)
scripts/brev-setup.shscripts/install.sh
|
Thanks for the contribution @dumko2001 — checksum verification for external downloads is a welcome security improvement. A few things to address before this is ready to merge: Stale version pin: The PR pins OpenShell to Checksum accuracy: CodeRabbit flagged a potential mismatch on one of the hashes. Please verify every DRY up the verification block: The same ~8-line checksum verification pattern is copy-pasted 3 times across the two scripts. A small Maintenance story: Hardcoded checksums are actually the right call here — downloading the upstream checksums file from the same source as the binary doesn't add real security, since a compromised release would include tampered checksums too. The ideal solution is GitHub artifact attestations, which would let us replace hardcoded hashes with Still need the rebase onto latest |
|
This is not a duplicate of #697 (ericksoa). Your approach — hardcoded SHA-256 hashes per architecture + version pinning — is strictly stronger than #697's approach of downloading We flagged the same-origin issue in our #697 review and filed OpenShell#542 requesting artifact attestations upstream. This PR could complement or replace #697's checksum approach. Worth rebasing onto current main — |
dumko2001
force-pushed
the
fix/h7-brev-binary-checksums
branch
from
48dc2af to
5b04d6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 356-363: The gh-based download currently can fail and cause the
script to exit (because of set -euo pipefail); modify the GH_TOKEN/gh release
download block so the gh invocation is attempted but its failure is caught and
triggers the fallback curl download: run GH_TOKEN="${GITHUB_TOKEN:-}" gh release
download "${OPENSHELL_VERSION}" --repo NVIDIA/OpenShell --pattern "$ASSET" --dir
"$tmpdir" and if that command returns non-zero then run the existing curl -fsSL
"https://github.com/NVIDIA/OpenShell/releases/download/${OPENSHELL_VERSION}/$ASSET"
-o "$tmpdir/$ASSET"; ensure you check the exit status (e.g., with an if ...;
then ...; else ...; fi) around the gh command and reference the variables
GH_TOKEN, OPENSHELL_VERSION, ASSET and tmpdir so the curl fallback executes on
gh failure.
- Around line 326-327: Update the pinned OpenShell release: change
OPENSHELL_VERSION from "v0.0.8" to "v0.0.14" and regenerate the three
EXPECTED_SHA values used for checksum verification; specifically fetch the
SHA-256 checksums for openshell-aarch64-apple-darwin.tar.gz,
openshell-x86_64-unknown-linux-musl.tar.gz, and
openshell-aarch64-unknown-linux-musl.tar.gz from the v0.0.14 release assets on
GitHub and replace the corresponding EXPECTED_SHA variables in the script so the
installer’s verification (the checksum comparison logic referencing
EXPECTED_SHA) matches the new release.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bcaa943-690f-4bff-ab25-7a342ce5fec0
📒 Files selected for processing (2)
scripts/brev-setup.shscripts/install.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/brev-setup.sh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.sh (1)
371-378: Checksum verification logic is correct; consider adding explicit failure message.The verification approach using
sha256sum -c/shasum -a 256 -cis correct. When a mismatch occurs, the tool outputs a "FAILED" message andset -eterminates the script. For improved user experience, you could wrap this in a conditional with an explicit security warning:💡 Optional: Add clearer error message on checksum failure
# Verify checksum if command -v sha256sum >/dev/null 2>&1; then - echo "${EXPECTED_SHA} $tmpdir/$ASSET" | sha256sum -c - + if ! echo "${EXPECTED_SHA} $tmpdir/$ASSET" | sha256sum -c - >/dev/null 2>&1; then + fail "Checksum verification failed for $ASSET. The binary may have been tampered with." + fi elif command -v shasum >/dev/null 2>&1; then - echo "${EXPECTED_SHA} $tmpdir/$ASSET" | shasum -a 256 -c - + if ! echo "${EXPECTED_SHA} $tmpdir/$ASSET" | shasum -a 256 -c - >/dev/null 2>&1; then + fail "Checksum verification failed for $ASSET. The binary may have been tampered with." + fi else fail "sha256sum or shasum not found. Cannot verify binary integrity." fi + info "Checksum verified"This also aligns with the PR review suggestion to extract a
verify_checksum()helper function for DRYness across this file andbrev-setup.sh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 371 - 378, Extract the checksum logic into a helper function named verify_checksum() and replace the inline block with a call to it; implement verify_checksum() to run sha256sum or shasum as currently done but capture the command exit status and, on failure, print an explicit security warning like "ERROR: checksum verification failed for $ASSET — aborting installation" (use process-failing behavior after the message), and return non-zero so set -e or caller can terminate. Ensure callers in this script and brev-setup.sh invoke verify_checksum() with EXPECTED_SHA, $tmpdir/$ASSET (or equivalent variables) so the DRY helper is reused.
🤖 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 371-378: Extract the checksum logic into a helper function named
verify_checksum() and replace the inline block with a call to it; implement
verify_checksum() to run sha256sum or shasum as currently done but capture the
command exit status and, on failure, print an explicit security warning like
"ERROR: checksum verification failed for $ASSET — aborting installation" (use
process-failing behavior after the message), and return non-zero so set -e or
caller can terminate. Ensure callers in this script and brev-setup.sh invoke
verify_checksum() with EXPECTED_SHA, $tmpdir/$ASSET (or equivalent variables) so
the DRY helper is reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84d93639-d780-4bd7-98ef-1c1ca15b1852
📒 Files selected for processing (2)
scripts/brev-setup.shscripts/install.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/brev-setup.sh
|
@cv thanks for the quick approval |
…s (upstream PR NVIDIA#177) Cherry-picked brev-setup.sh changes from dumko2001/fix/h7-brev-binary-checksums. Scripts/install.sh changes omitted — upstream replaced that file with a legacy wrapper; the original install logic has moved to root install.sh. - Pin openshell to v0.0.14 with SHA-256 checksum verification - Pin cloudflared to v2025.2.0 with SHA-256 checksums - Add gh fallback to curl for openshell download - Block macOS Intel (x86_64) installs explicitly Source: NVIDIA#177
cv
added
v0.0.13
|
Superseded by #697 and later installer changes: current main already verifies OpenShell download checksums, and the old brev-setup path this PR touched has since been removed. Closing instead of reviving a stale, conflicting branch. |
4 participants
Rationale
Downloading external binary assets without verification poses a security risk, as the assets could be tampered with during transit.
Changes
Integrated SHA-256 checksum verification into the asset download process to ensure the integrity of the downloaded binaries.
Verification Results
npm test.Leading Standards
This PR follows the project's 'First Principles' approach, prioritizing deterministic behavior and zero-trust security defaults.
Summary by CodeRabbit
Bug Fixes
Chores