fix(security): add SHA-256 integrity verification for Ollama installer#2048
Merged
fix(security): add SHA-256 integrity verification for Ollama installer#2048
Conversation
…r download verify_downloaded_script() computed a SHA-256 hash but only printed it — it never compared against an expected value. The Ollama installer was downloaded from https://ollama.com/install.sh and executed with no integrity verification, allowing MITM or CDN compromise to deliver arbitrary code. Pin the Ollama installer hash (OLLAMA_INSTALL_SHA256) and compare it before execution, matching the existing nvm verification pattern. Extend scripts/check-installer-hash.sh (from #2046) to also monitor the Ollama hash for upstream staleness via the weekly CI workflow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus 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 PR introduces SHA-256 hash verification for installer scripts across three components: a new GitHub Actions workflow that runs hash checks on a schedule, a bash script that validates pinned SHA-256 hashes against upstream sources, and enhanced installation logic that verifies downloaded scripts against expected hash values. Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions<br/>(Workflow)
participant CHS as check-installer-hash.sh
participant Upstream as Upstream<br/>Source
participant Repo as Repository<br/>Files
GHA->>CHS: Execute (with or without --update)
CHS->>Repo: Extract pinned hash
alt Hash exists
CHS->>Upstream: Download installer
Upstream-->>CHS: Installer script
CHS->>CHS: Compute SHA-256
alt --update flag
CHS->>Repo: Update pinned hash
CHS-->>GHA: Log UPDATED
else No --update flag
alt Hashes match
CHS-->>GHA: Log MATCH
else Hashes mismatch
CHS-->>GHA: Log STALE + exit 1
end
end
else Hash missing
CHS-->>GHA: Log SKIP
end
sequenceDiagram
participant User as User/<br/>Installer
participant Install as install.sh
participant Download as Download<br/>Mechanism
participant Verify as verify_downloaded_script()
User->>Install: Run installer
Install->>Download: Fetch installer script
Download-->>Install: Script downloaded
Install->>Verify: Call verify_downloaded_script(file, label, expected_hash)
alt expected_hash provided
Verify->>Verify: Compute SHA-256
alt Hash matches
Verify-->>Install: Verification passed
Install->>Install: Continue installation
else Hash mismatch
Verify->>Download: Remove file
Verify-->>Install: Exit with error
end
else No expected_hash
Verify->>Verify: Report SHA-256 (legacy behavior)
Verify-->>Install: Continue
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
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/check-installer-hash.sh`:
- Around line 65-68: The new registry registration calling register "NemoClaw
k8s installer" references the variable NEMOCLAW_INSTALLER_SHA256 required by
k8s/nemoclaw-k8s.yaml and currently causes CI to fail; either add the missing
SHA256 pin (set NEMOCLAW_INSTALLER_SHA256 to the correct hash and commit it) or
guard/defer the register call until the dependent branch is merged by wrapping
or conditionally executing the register invocation (the register invocation with
arguments "${REPO_ROOT}/k8s/nemoclaw-k8s.yaml", "NEMOCLAW_INSTALLER_SHA256",
"https://www.nvidia.com/nemoclaw.sh") behind a feature flag or branch-check so
CI does not require the variable yet.
- Around line 31-42: fetch_hash() uses sha256sum which is unavailable on macOS;
update fetch_hash to detect if sha256sum exists and, if not, fall back to using
shasum -a 256 (preserving the same output parsing to return only the hex
digest), e.g., try sha256sum first and otherwise call shasum -a 256 on the
temporary file created by mktemp and return the first field; keep the existing
trap and curl behavior and reference the fetch_hash function and its temporary
file handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: edf4f852-e0ec-4e27-99f0-e88d47d207af
📒 Files selected for processing (5)
.github/workflows/installer-hash-check.yamlMakefileinstall.shscripts/check-installer-hash.shscripts/install.sh
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/check-installer-hash.sh (1)
31-42:⚠️ Potential issue | 🟠 MajorAdd
shasum -a 256fallback infetch_hash().
fetch_hash()currently assumessha256sum; on macOS this often fails, causing the checker to abort before comparison/update. Mirror the fallback pattern already used inscripts/install.sh.♻️ Proposed fix
fetch_hash() { local url="$1" tmpfile tmpfile=$(mktemp) trap 'rm -f "$tmpfile"' RETURN @@ - sha256sum "$tmpfile" | cut -d' ' -f1 + if command -v sha256sum >/dev/null 2>&1; then + sha256sum "$tmpfile" | awk '{print $1}' + elif command -v shasum >/dev/null 2>&1; then + shasum -a 256 "$tmpfile" | awk '{print $1}' + else + echo "ERROR: No SHA-256 tool available (sha256sum/shasum)." >&2 + return 1 + fi }#!/bin/bash set -euo pipefail # Verify hashing-tool handling consistency between scripts. rg -n --type=sh 'fetch_hash|sha256sum|shasum' scripts/check-installer-hash.sh scripts/install.sh install.sh sed -n '31,45p' scripts/check-installer-hash.sh sed -n '136,162p' scripts/install.sh # Expected: check-installer-hash.sh should contain both sha256sum and shasum branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-installer-hash.sh` around lines 31 - 42, The fetch_hash() function currently assumes sha256sum and fails on macOS; modify fetch_hash to try sha256sum first and if that command is not available or fails, fall back to shasum -a 256 (same pattern used in scripts/install.sh), ensuring you handle errors/exit codes and still return only the hex digest; reference the function name fetch_hash and the utilities sha256sum and shasum -a 256 when making the change.
🧹 Nitpick comments (1)
scripts/check-installer-hash.sh (1)
49-53: Scope in-place updates to the target variable assignment.Current replacement is value-only; if the same digest appears elsewhere in the file, the wrong location can be rewritten. Update by variable name.
♻️ Proposed refactor
-update_pinned() { - local file="$1" old_hash="$2" new_hash="$3" - sed -i.bak "s/${old_hash}/${new_hash}/" "$file" +update_pinned() { + local file="$1" var_name="$2" new_hash="$3" + sed -i.bak -E "s#^([[:space:]]*${var_name}=\")[a-f0-9]{64}(\".*)#\1${new_hash}\2#" "$file" rm -f "${file}.bak" } @@ - update_pinned "$file" "$pinned" "$upstream" + update_pinned "$file" "$var" "$upstream"Also applies to: 102-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-installer-hash.sh` around lines 49 - 53, The replacement currently swaps any matching digest anywhere in the file; change update_pinned to target the specific variable assignment by adding a variable-name parameter and restricting the sed regex to only replace the assignment (e.g., match ^\s*VAR_NAME\s*=\s*"OLD_HASH" and replace with VAR_NAME="NEW_HASH"); update the function signature (update_pinned) and all call sites that invoke it (the other occurrences noted) to pass the variable name so only the intended assignment is updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/check-installer-hash.sh`:
- Around line 31-42: The fetch_hash() function currently assumes sha256sum and
fails on macOS; modify fetch_hash to try sha256sum first and if that command is
not available or fails, fall back to shasum -a 256 (same pattern used in
scripts/install.sh), ensuring you handle errors/exit codes and still return only
the hex digest; reference the function name fetch_hash and the utilities
sha256sum and shasum -a 256 when making the change.
---
Nitpick comments:
In `@scripts/check-installer-hash.sh`:
- Around line 49-53: The replacement currently swaps any matching digest
anywhere in the file; change update_pinned to target the specific variable
assignment by adding a variable-name parameter and restricting the sed regex to
only replace the assignment (e.g., match ^\s*VAR_NAME\s*=\s*"OLD_HASH" and
replace with VAR_NAME="NEW_HASH"); update the function signature (update_pinned)
and all call sites that invoke it (the other occurrences noted) to pass the
variable name so only the intended assignment is updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 254040db-99e6-423a-960a-5a9058008066
📒 Files selected for processing (1)
scripts/check-installer-hash.sh
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
macOS does not ship sha256sum — only shasum is available by default. fetch_hash() now tries sha256sum first, falls back to shasum -a 256, matching the pattern used in scripts/install.sh and install.sh. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-existing case statement indentation didn't match shfmt style. The CI "Files were modified by hooks" check runs shfmt on all touched files, so these must be formatted for the check to pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous formatting commit missed case statement body indentation (-ci flag) and binary operator line-continuation positioning (-bn flag) across all three shell files. This caused the "Files were modified by following hooks" CI check to fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
wscurran
added
security
The k8s/ directory and its associated hash-check script/workflow were removed on main (#2107). This PR's versions of check-installer-hash.sh and installer-hash-check.yaml are retained because they now serve the Ollama installer hash verification. Removed the NemoClaw k8s installer registry entry that referenced the deleted k8s/nemoclaw-k8s.yaml. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
cv
approved these changes
Apr 21, 2026
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
3 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
verify_downloaded_script()computed a SHA-256 hash but only printed it — never compared against an expected value. The Ollama installer was downloaded and executed with no integrity verification.OLLAMA_INSTALL_SHA256and compares before execution, matching the existing nvm verification pattern.scripts/check-installer-hash.sh(from fix(security): add SHA-256 integrity verification for k8s installer download #2046) into a registry-based multi-hash checker that monitors both the k8s NemoClaw installer and the Ollama installer for upstream staleness.make check-installer-hash.Ordering dependency
Files changed
scripts/install.shverify_downloaded_script()accepts optional expected hash;OLLAMA_INSTALL_SHA256constant; both Ollama download sites pass hashinstall.shscripts/check-installer-hash.sh.github/workflows/installer-hash-check.yamlMakefilemake check-installer-hashtargetTest plan
OLLAMA_INSTALL_SHA256to a wrong value — installer aborts with "integrity check failed", Ollama script does NOT executescripts/check-installer-hash.sh— reports both hashes OKscripts/check-installer-hash.sh --updatewith a stale hash — rewrites the correct file🤖 Generated with Claude Code
Summary by CodeRabbit