feat(security): verify binary SHA-256 checksum on download (B5)#15
feat(security): verify binary SHA-256 checksum on download (B5)#15
Conversation
- Fetch checksums.txt from release assets after binary download - Compute SHA-256 of downloaded binary and compare - On mismatch: delete binary and raise RuntimeError with clear message - Graceful fallback: warn if checksums.txt not available (older releases) Addresses: design-partner-eval B5 (P1 security)
|
✅ All checks passed! Ready for review. |
There was a problem hiding this comment.
Pull request overview
Adds SHA-256 integrity verification to the capiscio-core binary download flow in capiscio-python, to help ensure the downloaded executable matches the published release artifact.
Changes:
- Fetch
checksums.txtfor a given GitHub release and extract the expected checksum for the platform binary. - Compute the downloaded binary’s SHA-256 and compare it to the expected checksum; delete and error on mismatch.
- Warn and continue when checksums cannot be obtained.
| import sys | ||
| import hashlib | ||
| import platform | ||
| import stat | ||
| import shutil |
There was a problem hiding this comment.
sys and shutil appear 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.
| import sys | |
| import hashlib | |
| import platform | |
| import stat | |
| import shutil | |
| import hashlib | |
| import platform | |
| import stat |
| 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 | ||
|
|
There was a problem hiding this comment.
_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 |
| logger.warning(f"Binary {filename} not found in checksums.txt") | ||
| return None |
There was a problem hiding this comment.
If checksums.txt is successfully fetched but doesn’t contain an entry for the expected binary filename, the code currently logs a warning and disables verification by returning None. Given the file exists, missing the entry is suspicious and should likely be treated as an integrity failure (abort install) rather than silently proceeding with an unverified binary.
| 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}" | |
| ) |
| # Verify checksum integrity | ||
| expected_hash = _fetch_expected_checksum(version, filename) | ||
| if expected_hash is not None: | ||
| if not _verify_checksum(target_path, expected_hash): | ||
| target_path.unlink() |
There was a problem hiding this comment.
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).
Summary
Adds SHA-256 checksum verification for downloaded capiscio-core binaries (B5 - design partner eval, part 2/3).
Changes
checksums.txtfrom the same releaseRuntimeErrorwith a clear error messagechecksums.txtis not available (older releases), log a warning and continueRelated PRs
checksums.txtin releaseEvaluation Plan
Design partner eval item B5 (P1 — Security)