-
Notifications
You must be signed in to change notification settings - Fork 0
feat(security): verify binary SHA-256 checksum on download (B5) #15
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import hashlib | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import platform | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import stat | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import shutil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -68,6 +69,34 @@ def get_binary_path(version: str) -> Path: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # For now, let's put it in a versioned folder | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return get_cache_dir() / version / filename | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _fetch_expected_checksum(version: str, filename: str) -> Optional[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Fetch the expected SHA-256 checksum from the release checksums.txt.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| url = f"https://github.com/{GITHUB_REPO}/releases/download/v{version}/checksums.txt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resp = requests.get(url, timeout=30) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resp.raise_for_status() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for line in resp.text.strip().splitlines(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parts = line.split() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(parts) == 2 and parts[1] == filename: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return parts[0] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"Binary {filename} not found in checksums.txt") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+82
to
+83
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"Binary {filename} not found in checksums.txt") | |
| return None | |
| # If checksums.txt was fetched successfully but does not contain an entry | |
| # for the expected binary filename, treat this as an integrity failure | |
| # rather than silently proceeding without verification. | |
| logger.error(f"Binary {filename} not found in checksums.txt") | |
| raise RuntimeError( | |
| f"Integrity check failed: expected checksum entry for {filename} " | |
| f"not found in checksums.txt for version v{version}" | |
| ) |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_fetch_expected_checksum treats any RequestException (timeouts, DNS issues, connection errors, etc.) as “checksums not available” and continues without verification. That makes the integrity check bypassable if an attacker or network issue blocks the checksum fetch. Consider only falling back when the HTTP status is 404/410, and otherwise failing closed (or making this behavior explicitly configurable).
| for line in resp.text.strip().splitlines(): | |
| parts = line.split() | |
| if len(parts) == 2 and parts[1] == filename: | |
| return parts[0] | |
| logger.warning(f"Binary {filename} not found in checksums.txt") | |
| return None | |
| except requests.exceptions.RequestException as e: | |
| logger.warning(f"Could not fetch checksums.txt: {e}") | |
| return None | |
| except requests.exceptions.HTTPError as e: | |
| status_code = getattr(e.response, "status_code", None) | |
| # Treat missing checksum file as "no checksum available" and continue without verification. | |
| if status_code in (404, 410): | |
| logger.warning(f"checksums.txt not found at {url} (status {status_code}); " | |
| "continuing without checksum verification.") | |
| return None | |
| # For other HTTP errors, fail closed. | |
| logger.error(f"Failed to fetch checksums.txt from {url}: {e}") | |
| raise RuntimeError("Unable to fetch checksums for capiscio-core binary.") from e | |
| except requests.exceptions.RequestException as e: | |
| # Network/transport errors (timeouts, DNS issues, etc.) should cause a hard failure. | |
| logger.error(f"Network error while fetching checksums.txt from {url}: {e}") | |
| raise RuntimeError("Network error while fetching checksums for capiscio-core binary.") from e | |
| for line in resp.text.strip().splitlines(): | |
| parts = line.split() | |
| if len(parts) == 2 and parts[1] == filename: | |
| return parts[0] | |
| logger.warning(f"Binary {filename} not found in checksums.txt") | |
| return None |
Copilot
AI
Mar 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New behavior (fetching checksums.txt, computing SHA-256, and failing on mismatch) isn’t covered by tests in this PR. Since this code path is security-critical and also adds a second requests.get call, please add/update unit tests to cover: checksum file present + match, present + mismatch (deletes file + raises), and missing checksums (warns + continues).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sysandshutilappear to be unused in this module. Since this PR is already touching the import block, consider removing unused imports to avoid lint/IDE warnings and keep dependencies clear.