fix: add CAPISCIO_REQUIRE_CHECKSUM fail-closed mode (B5 hardening)#16
fix: add CAPISCIO_REQUIRE_CHECKSUM fail-closed mode (B5 hardening)#16
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in fail-closed mode for core binary checksum verification during download, improving supply-chain hardening while keeping the default behavior backward compatible.
Changes:
- Add checksum fetching from
checksums.txtand SHA-256 verification of the downloaded binary. - Introduce
CAPISCIO_REQUIRE_CHECKSUM=trueto enforce fail-closed behavior when checksums can’t be validated. - Add logging and error handling around checksum verification and cleanup.
src/capiscio/manager.py
Outdated
| 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 | ||
| 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() creates a requests.get() Response without closing it. Please use a context manager (with requests.get(...) as resp:) or explicitly resp.close() to avoid leaking connections/file descriptors under repeated downloads.
src/capiscio/manager.py
Outdated
| f"but checksums.txt is not available for v{version}. " | ||
| "Cannot verify binary integrity." | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| "Could not verify binary integrity (checksums.txt not available). " |
There was a problem hiding this comment.
When CAPISCIO_REQUIRE_CHECKSUM=true and the binary is missing from checksums.txt, _fetch_expected_checksum() returns None, but the raised error says checksums.txt is not available. This is misleading; distinguish between (a) checksums file fetch failure and (b) checksum entry missing for the asset, and tailor the error message accordingly.
| f"but checksums.txt is not available for v{version}. " | |
| "Cannot verify binary integrity." | |
| ) | |
| else: | |
| logger.warning( | |
| "Could not verify binary integrity (checksums.txt not available). " | |
| f"but no checksum entry is available for {filename} in checksums.txt " | |
| f"for v{version}. Cannot verify binary integrity." | |
| ) | |
| else: | |
| logger.warning( | |
| f"Could not verify binary integrity for {filename} " | |
| f"(no checksum entry found in checksums.txt for v{version}). " |
src/capiscio/manager.py
Outdated
| # Verify checksum integrity | ||
| require_checksum = os.environ.get("CAPISCIO_REQUIRE_CHECKSUM", "").lower() in ("1", "true", "yes") | ||
| expected_hash = _fetch_expected_checksum(version, filename) | ||
| if expected_hash is not None: | ||
| if not _verify_checksum(target_path, expected_hash): | ||
| target_path.unlink() | ||
| raise RuntimeError( | ||
| f"Binary integrity check failed for {filename}. " | ||
| "The downloaded file does not match the published checksum. " | ||
| "This may indicate a tampered or corrupted download." | ||
| ) | ||
| logger.info(f"Checksum verified for {filename}") | ||
| elif require_checksum: | ||
| target_path.unlink() | ||
| raise RuntimeError( | ||
| f"Checksum verification required (CAPISCIO_REQUIRE_CHECKSUM=true) " | ||
| f"but checksums.txt is not available for v{version}. " | ||
| "Cannot verify binary integrity." | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| "Could not verify binary integrity (checksums.txt not available). " | ||
| "Set CAPISCIO_REQUIRE_CHECKSUM=true to enforce verification." | ||
| ) |
There was a problem hiding this comment.
Checksum verification adds several new behaviors/branches (verified match, mismatch cleanup + error, require-checksum fail-closed when checksums are unavailable or missing). tests/unit/test_manager.py currently doesn't cover these paths; please add unit tests that mock checksum fetch results and assert the correct failure mode and cleanup behavior.
src/capiscio/manager.py
Outdated
| st = os.stat(target_path) | ||
| os.chmod(target_path, st.st_mode | stat.S_IEXEC) | ||
|
|
||
| # Verify checksum integrity |
There was a problem hiding this comment.
CAPISCIO_REQUIRE_CHECKSUM is a new user-facing environment variable that changes security posture (fail-open vs fail-closed). It doesn't appear to be documented in the repo’s README/docs; please add a brief note in the user docs so operators know how to enable it and what to expect when checksums are unavailable.
| # Verify checksum integrity | |
| # Verify checksum integrity. | |
| # | |
| # CAPISCIO_REQUIRE_CHECKSUM (env var): | |
| # - When set to a truthy value ("1", "true", "yes"), checksum verification | |
| # is required (fail-closed). If a published checksums.txt file is not | |
| # available for the requested version, the downloaded binary is deleted | |
| # and installation fails with an error. | |
| # - When unset or falsey (default), checksum verification is best-effort | |
| # (fail-open). If checksums are unavailable, a warning is logged and the | |
| # binary is kept. |
1. Close requests.get response with context manager in _fetch_expected_checksum 2. Return (checksum, status) tuple to distinguish fetch_failed vs entry_missing 3. Tailor error messages for each failure mode 4. Document CAPISCIO_REQUIRE_CHECKSUM env var behavior in code comment 5. Move chmod +x after checksum verification (verify before trust) 6. Add unit tests for all checksum paths (match, mismatch, require+fetch_failed, require+entry_missing)
When CAPISCIO_REQUIRE_CHECKSUM=true, binary download fails if checksums.txt is unavailable or the asset is missing from it. Default behavior remains fail-open (warn and continue) for backward compatibility.
1. Close requests.get response with context manager in _fetch_expected_checksum 2. Return (checksum, status) tuple to distinguish fetch_failed vs entry_missing 3. Tailor error messages for each failure mode 4. Document CAPISCIO_REQUIRE_CHECKSUM env var behavior in code comment 5. Move chmod +x after checksum verification (verify before trust) 6. Add unit tests for all checksum paths (match, mismatch, require+fetch_failed, require+entry_missing)
345725a to
829d86b
Compare
|
✅ All checks passed! Ready for review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/unit/test_manager.py:176
test_downloads_binary_on_missingimplicitly relies on the ambientCAPISCIO_REQUIRE_CHECKSUMenv var being unset/false. If a developer or CI sets it globally, this test will start failing (since the patched_fetch_expected_checksumreturnsfetch_failedand strict mode will raise). Consider explicitly patchingos.environto clear or setCAPISCIO_REQUIRE_CHECKSUMto a falsey value for this test (and any other tests that expect fail-open behavior).
@patch('capiscio.manager._fetch_expected_checksum', return_value=(None, "fetch_failed"))
@patch('capiscio.manager.get_platform_info', return_value=('linux', 'amd64'))
@patch('capiscio.manager.get_binary_path')
@patch('capiscio.manager.requests.get')
@patch('capiscio.manager.console')
def test_downloads_binary_on_missing(self, mock_console, mock_requests, mock_get_path, mock_platform, mock_fetch_checksum):
"""Test that binary is downloaded when missing."""
mock_path = MagicMock(spec=Path)
mock_path.exists.return_value = False
mock_path.parent = MagicMock()
mock_path.name = "capiscio-linux-amd64"
mock_get_path.return_value = mock_path
# Mock the response
mock_response = MagicMock()
mock_response.headers = {'content-length': '1024'}
mock_response.iter_content.return_value = [b'x' * 1024]
mock_response.__enter__ = MagicMock(return_value=mock_response)
mock_response.__exit__ = MagicMock(return_value=False)
mock_requests.return_value = mock_response
with patch('builtins.open', mock_open()):
with patch.object(os, 'stat') as mock_stat:
with patch.object(os, 'chmod'):
mock_stat.return_value = MagicMock(st_mode=0o644)
result = download_binary("1.0.0")
Adds
CAPISCIO_REQUIRE_CHECKSUM=trueenv var that makes binary download fail-closed when checksums.txt is unavailable or the asset is missing from it. Default behavior remains fail-open for backward compatibility.Part of design partner re-evaluation B5 hardening.